Prompt responses#207
Conversation
|
Thanks for putting this together @JRoush, I hope you'll forgive me if I take some time to make sure it gets a thorough review. Can I ask for a couple of annoying procedural tweaks first of all, while I actually review the code?
|
These test the desired behavior of the library; they do not all currently pass.
On reflection, this its not really possible for this test to pass. The server can't (or rather, shouldn't) read the request body until it has sent a `100 continue` response. But it cant send a continue response until it has finished sending the _full_ response to the previous request in the pipeline, because responses have to be sent in the correct order. This forced interleaving of reads and writes prevents prompt request handling.
The server must parse all pipelined requests (sent over a single connection) sequentially. Dropping the internal reader when it has reached the end of the request signals that the next request can begin parsing. This can happen concurrently while a response to the first request is being sent. Dropping a request reader _before_ it has been fully read will still consume the rest of the request, blocking if necessary until all of the data arrives. This ensures that the body of a request is always read eventually, unless the connection is closed by the client. However, request readers are no longer forcibly dropped before sending a response (they are instead dropped _after_ sending a response). This reverts 091ce80. Responses can now be sent without reading the full request body, which is very useful for e.g. "413 Payload too large" errors. Server authors should be advised to fully read the request body themselves before sending lengthy responses.
bradfier
left a comment
There was a problem hiding this comment.
This looks good to me barring the one line in the test suite that breaks under our MSRV. Can we swap that out for a clunkier alternative and I'll get this merged?
| fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> { | ||
| sleep(Duration::from_millis(100)); | ||
| let l = self.len.min(buf.len()).min(1000); | ||
| buf[..l].fill(self.val); |
There was a problem hiding this comment.
I appreciate this is just inside test code but it breaks under 1.41.1 because .fill() wasn't yet implemented for [u8].
https://github.com/tiny-http/tiny-http/pull/207/checks?check_run_id=2940126934
|
OK, hopefully the CI tests will pass now. Thanks for your patience @bradfier. I haven't used github workflows before and it took a while to figure out how to run them on my fork. |
Thanks! It looks good now, however it appears Travis CI have finally thrown open source under the bus like they said they would, and stopped travis-ci.org builds from running at all. I've asked Tomaka and Frewsxcv to remove the Travis CI gate from the project via EMail as they probably have notifications from this repo turned off. |
Allows the server to send responses immediately, without waiting for the full request body to arrive.
The trade-off is that the server author must explicitly read the full request body before responding if they want the next request in the pipeline to start parsing concurrently while the response is being sent.
This is intended to resolve issue #206.