Fix order of internal message buffer

stepancheg / grpc-rust

Rust implementation of gRPC

MIT License

1.37k stars 124 forks source link

Fix order of internal message buffer #57

Closed weiznich closed 7 years ago

weiznich commented 7 years ago

In case of a large streamed rpc call the connection broke at ~64kb, because grpc tried to renew the http stream. In this case there is more than one message in the internall buffer. The old code pushed new message to the front of the buffer and also poped them from front. If there is more than one message in this buffer this will result in a invalid order.

stepancheg commented 7 years ago

I managed to reproduce the problem with large messages.

I believe, the cause was different. I found a problem and fixed it.

Please reopen the issue if problem persists.

Thanks for the bugreport.

weiznich commented 7 years ago

Without this patch I'm seeing unexpected wire type WireTypeLengthDelimited errors popping while sending a stream from client to server, that exceeded the window size (containing more than ~65kb of data). For me this is not fixed with 49b6c83 (Could not reopen the pull request, so this should be submitted as a new pull request?)

stepancheg commented 7 years ago

I believe the code which is affected by your patch is correct: you push items to the back, and pop from front. Except one case where you push to front truncated element.

So if you are sure your patch works, could you please explain to me where I am wrong?

Or I need to spend some time to reproduce the problem.

weiznich commented 7 years ago

The problem is there are places where code pushes to the front of the queue (for example in client_conn.rs). I did not look further around when creating this path, so I changed the order in http_common.rs. After looking at the other code it may be a better idea to change the push in client_conn.rs (This also fixes the issue for me).

stepancheg commented 7 years ago

Thanks! I overlooked that.