Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IPC transport bug: Use write_all to send data over socket #648

Merged
merged 3 commits into from
Mar 27, 2023

Conversation

Bobface
Copy link
Contributor

@Bobface Bobface commented Jun 18, 2022

Data is sent over the IPC socket using .write() on a UnixStream. On success this method returns the number of bytes written as integer. This number is not handled however, the result is only checked for success.

Documentation for write() (highlighted important parts):

Writes a buffer into this writer, returning how many bytes were written.

This function will attempt to write the entire contents of buf, but the entire write may not succeed, or the write may also generate an error. A call to write represents at most one attempt to write to any wrapped object.

This seems to rarely be an issue with "normal" usage of web3-rs, since the messages sent over the socket are relatively small, a few hundred bytes at maximum. This seems to always fully succeed.

Just recently however, I started sending larger requests to my Ethereum node via an IPC socket and began hitting a bug where the node would randomly close the connection due to JSON parsing errors. After some debugging, it turns out that when sending large messages, they might not be sent in full due to not handling the return value of write().

This PR fixes this bug by replacing .write() with .write_all():

Attempts to write an entire buffer into this writer.

This method will continuously call write until there is no more data to be written. This method will not return until the entire buffer has been successfully written or such an error occurs. The first error generated from this method will be returned.

@Bobface
Copy link
Contributor Author

Bobface commented Jul 11, 2022

Pinging this, should be a quick fix :).

@tomusdrw tomusdrw enabled auto-merge (squash) March 27, 2023 20:35
@tomusdrw tomusdrw merged commit 68f2a6d into tomusdrw:master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants