-
-
Notifications
You must be signed in to change notification settings - Fork 47
Make fail_request properly finish the response #129
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
Conversation
@@ -14,6 +15,9 @@ class Server < Connection | |||
def fail_request(status) | |||
@persistent = false | |||
write_response(@version, status, {}, nil) | |||
write_body(@version, nil) | |||
rescue Errno::ECONNRESET, Errno::EPIPE | |||
# Handle when connection is already closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have a test case for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the test context 'multiple client requests' from shared_examples.rb
does exercise it already. In that particular test, there's also an original ECONNRESET
that was the catalyst for the fail_request
call to begin with, and that one is still re-raised by next_request
.
I managed to trigger the issue in fail_request
via other means originally (hence why I've included it in this PR), but I don't remember what combination of events caused it and can't find it in my notes. It also could have been because it was adding another nested layer to whatever original exception and made things slightly more difficult to debug.
Does that give you enough confidence to go with this as-is?
If you're amenable, I might submit a future PR to rescue more exceptions in some other spots so as to reduce the frequency of tasks ending with unhandled exceptions. Even in the case of the 'multiple client requests' test above, I believe it will require both the rescue
included here as well as these other future rescues to fully absorb all these exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and the repush just adds log silencing since I just stumbled onto your pattern for doing so. The rest is unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
f64fa68
to
e58bbe2
Compare
Previously
fail_request
wrote the initial part of the response, but failed to finish it. When tested withAsync::HTTP::Client
, this caused the client to experience anEOFError
.This PR causes
fail_request
to now finish the response. It also rescues exceptions that might arise from the attempt to write the response.Before:
After:
Types of Changes
Contribution