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

Chunked Transfer-Encoding + bincode causes occasional unrelated request failures #147

Open
aidanhs opened this issue Jul 15, 2018 · 0 comments

Comments

@aidanhs
Copy link

aidanhs commented Jul 15, 2018

This is the observed behaviour when using bincode + chunked encoding:

  • make request with bincode body
  • request is read by user code, ok response is built by user code and sent by tiny_http
  • 400 response is sent by tiny_http (no request having been sent to cause it)
  • tiny-http closes connection

This happens on every single request when using chunked bincode

The cause of this is that, unlike serde_json (which explicitly consumes the rest of the reader to make sure there's only whitespace - https://github.com/serde-rs/json/blob/v1.0.23/src/de.rs#L2122, https://github.com/serde-rs/json/blob/v1.0.23/src/de.rs#L125-L133), bincode is happy just to consume the part of the reader it needs (https://github.com/TyOverby/bincode/blob/v1.0.1/src/internal.rs#L75-L84). Therefore when deserializing bincode, you end up with a trailing 0\r\n as part of chunked encoding that you haven't looked at (unless you explicitly try and read more bytes, and why would you?).

When the response is sent and the reader is dropped, we then forget that we were in the middle of an incomplete chunked body, and so the 0 is read as a request line of the next request, which is nonsense. tiny_http then spits out a 400 response and closes the connection.

Generally you might not notice since a pooling http requester (like reqwest) will throw away closed connections and ignore the unexpected 400. However, if you are making huge numbers of requests very quickly and so start sending a request before the connection is closed, then it will look like you've got a 400 response for no reason (and there's no detail in the 400 response to explain what's going on).

I think there are a few aspects that could be improved here:

  • the chunked decoder should (if a chunk has been completed) read the size of the next chunk so that if you hit eof of readable data you know you've hit eof of tcp data
  • the 400 responses generated internally by tiny_http (https://github.com/tiny-http/tiny-http/blob/0.6.0/src/client.rs#L187-L201) should contain some information that these are internal errors and the http request is invalid/wrong in some way
  • the more general problem of "what happens if you throw away a request that has not had its body read" should be addressed. I see two solutions (a. reading the rest on a background thread, b. closing the connection) and I think b is probably the better one. The current situation of "it hangs around to corrupt the next request" seems subideal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants