Extend the http test for partially read reply body #2780
Conversation
This is to make next patching a bit simpler Signed-off-by: Pavel Emelyanov <[email protected]>
When closing server-side connection it may happen that client had already closed its end and there's nothing to flush into. Neither there's anything that can be done about it. Ignoring exception here is safe, all the test wants to check is done on the client side, server just supplies the reply at its best. refs: 303d0cb Signed-off-by: Pavel Emelyanov <[email protected]>
Extend the test with partially-read body to verify the same for chunked-encoded replies. For now the check fails, because client only tracks under-read bodies for content-length replies. This is to be fixed on top. Signed-off-by: Pavel Emelyanov <[email protected]>
xemul
force-pushed
the
br-http-client-partial-chunked-body-read-test
branch
from
4f4da63 to
dc1270b
Compare
|
upd:
|
| } | ||
| } | ||
| out.close().get(); | ||
| out.close().handle_exception([](auto ex){}).get(); |
There was a problem hiding this comment.
how come after just splitting test to function and the test close() started to throw? Am I missing something?
There was a problem hiding this comment.
or it is "test/http: Extend test for improper client handling of aborted requests" would introduce exception throwing?
There was a problem hiding this comment.
It may happen that server "survives" up until the very last flush. Then, since this last flush schedules a batch flush, the exception is set and delivered only on close()
There was a problem hiding this comment.
It's great that you are writing a test to rerproduce a bug (#2768), but I don't think it's a good idea to have tests that enshrine incorrect behavior. I think the test should check the correct behavior, and fail. Ideally we should be able able to mark this test "xfail" ("expected failure") as we can do in pytest - but I don't know how. Another ugly option is just to #if 0 the test out :-(
| } else { | ||
| // FIXME chunked encoded bodies are not yet checked by client to be | ||
| // under-read, so 2nd request will receive the tail of the 1st response | ||
| BOOST_REQUIRE_THROW(make_request.get(), httpd::response_parsing_exception); |
There was a problem hiding this comment.
I don't like to have tests which enshrine wrong behavior. I'd much rather have a test that should pass, and for now we know fails (i.e., "xfail" in pytest lingo). I don't know how to do this in unit tests.
But it still doesn't mean we can have a test which insists that a known bug can't be fixed...
| [&content_length](const http::reply& resp, input_stream<char>&& in) { | ||
| content_length = resp.content_length; | ||
| [&content_length, chunked](const http::reply& resp, input_stream<char>&& in) { | ||
| content_length = chunked ? 128_KiB : resp.content_length; |
There was a problem hiding this comment.
Where does this 128KB come from? Response chunks have a length written by the server - the client shouldn't guess them. I see below that your server indeed returns the entire response as one chunk - and writes the chunk size in the beginning of the chunk.
| auto ss = lcf.get_server_socket(); | ||
| auto server = ss.accept().then(make_response); | ||
| if (size > 128_KiB) { | ||
| if (size > 128_KiB && !chunked) { |
|
Closing in favor of #3063 |
3 participants
There's a test that highlights an issue in the client's behavior when a socket is closed before all request data is fully read. When this happens, the subsequent request shouldn't receive leftover data from the previous response and fail. For content-length replies this case is handled by client and the test passes. For chunked-encoded bodies, the problem is yet to be fixed.
This test extension is intended solely to demonstrate the problem and does not include a fix. The resolution will be addressed in a follow-up PR.
ref #2767
ref #2768