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.

Sign up for GitHub

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
Merged

Make fail_request properly finish the response #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/async/http/protocol/http1/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# Released under the MIT License.
# Copyright, 2018-2023, by Samuel Williams.
# Copyright, 2020, by Igor Sidorov.
# Copyright, 2023, by Thomas Morgan.

require_relative 'connection'

Expand All @@ -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.

end

def next_request
Expand Down
20 changes: 20 additions & 0 deletions spec/async/http/protocol/http11_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,33 @@
# Released under the MIT License.
# Copyright, 2017-2023, by Samuel Williams.
# Copyright, 2018, by Janko Marohnić.
# Copyright, 2023, by Thomas Morgan.

require 'async/http/protocol/http11'
require_relative 'shared_examples'

RSpec.describe Async::HTTP::Protocol::HTTP11 do
it_behaves_like Async::HTTP::Protocol

context 'bad requests' do
include_context Async::HTTP::Server

around do |example|
current = Console.logger.level
Console.logger.fatal!

example.run
ensure
Console.logger.level = current
end

it "should fail cleanly when path is empty" do
response = client.get("")

expect(response.status).to be == 400
end
end

context 'head request' do
include_context Async::HTTP::Server

Expand Down