Add test_client_quick_close_no_lowlevel_error_handler_call [changelog skip] #2429
Conversation
test/test_puma_server.rb
Outdated
|
|
||
| sock = TCPSocket.new @host, @port | ||
| # normal request doesn't generate EOFError | ||
| sock.syswrite "GET / HTTP" |
There was a problem hiding this comment.
Since this test isn't specifically testing malformed HTTP requests, it'd be nice to make sure that this one is well-formed.
sock.syswrite "GET / HTTP/1.1\r\n\r\n"There was a problem hiding this comment.
Let me check, but I don't think that threw the error in 5.0.2. I started with a normal request and a read, but nothing was raised. I think.
There was a problem hiding this comment.
Oh I think I understand your comment normal request doesn't generate EOFError then.
That's interesting. That means that this test isn't reproducing exactly the same kinda thing we see with curl as in #2390, but instead found a different way to cause the EOFError reliably.
There was a problem hiding this comment.
Oh I think I understand your comment normal request doesn't generate EOFError then.
But just for you I checked things.
As long as the request 'write' is valid, no error, with or without a read. So, with 5.0.2, as the test is, the failure shows
Expected "LLEH EOFError" to be empty.
So 'EOFError' is the err.message, and the class is the same. I had to add the delay, as it passed without it...
There was a problem hiding this comment.
Thanks for checking! That makes sense.
As I understand it, this test does not test the exact same regression described in #2390 , but does test for another, closely-related regression that was introduced in 5.0.1.
The test as written ensures that if a client closes the connection before writing a complete HTTP request, that puma doesn't invoke the low-level error handler. That's a good test to have.
A test that would more closely simulate what's described in #2390 would have the client write a complete HTTP request to the socket before closing it, but if we modify the test as written to do that, it doesn't reliably reproduce the problem for some reason. If we figure out how to do that in the future, it'd be great to have, but for now this test is a good substitute.
There was a problem hiding this comment.
Maybe add another socket that writes a normal requests reads the response and closes immediately? Or maybe two more, such that:
Socket 1 - writes a valid request, reads it, closes immediately
Socket 2 - writes a valid request, closes immediately
Socket 3 - writes an invalid request, closes immediately
At the end of that, @events.stdout.string should still be empty?
There was a problem hiding this comment.
Yeah, all of those three cases are cases where we'd expect that low-level error handler to not be invoked, but for all three cases, it actually was invoked on puma 5.0.1. I could see each of those three being valuable test cases.
But so far we're only able to write a test that reliably demonstrates the failure for the socket 3 case. We don't have red tests for the first and second cases.
There was a problem hiding this comment.
But so far we're only able to write a test that reliably demonstrates the failure for the socket 3 case
Correct, at least in 5.0.2. I updated to use three cases. Given that all should pass, might as well. Having been bitten by 'that will never break, so why write a test?', no harm in adding them...
| sock.close | ||
|
|
||
| sleep 0.7 # need some delay for the lleh to be called | ||
| assert_empty @events.stdout.string |
There was a problem hiding this comment.
How do you feel about making an additional assertion that the client received a successful response?
There was a problem hiding this comment.
In the other thread, @MSP-Greg demonstrated that the request has to be malformed, so the client won't get a successful response. Doesn't make sense to test for it in this case.
|
LGTM. Thanks for adding this! |
Description
See #2390
Fixed by #2279
Fails in 5.0.2, showing EOFError being thrown.
Your checklist for this pull request
[changelog skip]or[ci skip]to the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.