Skip to content

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

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Aug 25, 2023

Previously fail_request wrote the initial part of the response, but failed to finish it. When tested with Async::HTTP::Client, this caused the client to experience an EOFError.

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:

HTTP/1.1 400 Bad Request

After:

HTTP/1.1 400 Bad Request
connection: close
content-length: 0
                  <- required final blank line

Types of Changes

  • Bug fix.

Contribution

@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

@ioquatix ioquatix merged commit c58bafb into socketry:main Aug 25, 2023
@zarqman zarqman deleted the fix-fail-request branch August 25, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants