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

parse_length_delimited_from_reader doesn't work for TcpStream unless other side calls shutdown(Shutdown::Write) #157

Closed
luser opened this issue May 12, 2016 · 6 comments

Comments

@luser
Copy link

luser commented May 12, 2016

I tried to use parse_length_delimited_from_reader for parsing a protobuf Message from a TcpStream, but I think it calls read too many times on the underlying TcpStream and winds up blocking unless the other side calls shutdown(Shutdown::Write). I put together a fairly simple example client/server that demonstrates the problem:
https://github.com/luser/rust-protobuf-network-example/blob/master/src/bin/client.rs
https://github.com/luser/rust-protobuf-network-example/blob/master/src/bin/server.rs
https://github.com/luser/rust-protobuf-network-example/blob/master/proto.proto

With the calls to stream.shutdown() commented out, the client hangs waiting for a response from the server, and the server hangs waiting for more data from the client. With those calls uncommented, everything works.

For this simple case, this workaround would be sufficient, but if I want to have the client and server exchange multiple messages this will not work. I wound up having to manually parse the length, call read_exact, and then call parse_from_bytes, which is quite a bit more code:
https://github.com/luser/sccache2/blob/d9e5315b9da649112ad08c5b15016b13c9a4f5ba/src/client.rs#L51

It seems like in the parse_length_delimited case, rust-protobuf ought to be able to do just this and not attempt to read more bytes than it needs, which would make this scenario much simpler to write.

@riemervdzee
Copy link

I am running against the same problem but unfortunately I cannot close the stream on the server. Are there any ideas on how to fix this in this library/crate? I might have some time available to attempt to implement/test something

@stepancheg
Copy link
Owner

I don't know why that issue can happen.

If it's possible, could you please provide a small standalone example which reproduces the problem?

@fzgregor
Copy link

fzgregor commented Oct 9, 2017

I debugged this once some time ago. Sorry, for the vague report.

The problem is that the library invokes .eof() on the stream to check whether there is more data to read. The stream was previously primed with the length of the message parsed from the varint in front of the message. However, the check_eof() - or eof(), I don't remember - tries to read from the underlying stream again when it realizes that it read limit bytes. This last call leads the stream to block for incoming traffic.

I fell back to manually read the message into a buffer and parse it from there (which is obviously not that nice, but it works).

@stepancheg
Copy link
Owner

Probably you are correct. I believe bug is inside BufReadIter.eof() when it is already at limit and at the end of buffer. I need to spend time to reproduce a problem.

@stepancheg
Copy link
Owner

Hope I fixed it in 99c4279. I need to think about it again, and then I'll release an update.

stepancheg added a commit that referenced this issue Oct 14, 2017
Otherwise synchronous reads may block.

Fixes #157
@stepancheg
Copy link
Owner

Published rust-protobuf 1.4.2.

jxs pushed a commit to jxs/rust-protobuf that referenced this issue Oct 25, 2022
Added option to set `#[repr(...)]` on generated types.
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

No branches or pull requests

4 participants