Skip to content
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

Fix rack.after_reply exceptions breaking connections #2861

Merged
merged 2 commits into from Apr 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/puma/request.rb
Expand Up @@ -182,7 +182,11 @@ def handle_request(client, lines, requests)
res_body.close if res_body.respond_to? :close
end

after_reply.each { |o| o.call }
begin
after_reply.each { |o| o.call }
rescue StandardError => e
@log_writer.debug_error e
end
end

res_info[:keep_alive]
Expand Down
50 changes: 48 additions & 2 deletions test/test_rack_server.rb
Expand Up @@ -36,8 +36,8 @@ def call(env)
def setup
@simple = lambda { |env| [200, { "X-Header" => "Works" }, ["Hello"]] }
@server = Puma::Server.new @simple
port = (@server.add_tcp_listener "127.0.0.1", 0).addr[1]
@tcp = "http://127.0.0.1:#{port}"
@port = (@server.add_tcp_listener "127.0.0.1", 0).addr[1]
@tcp = "http://127.0.0.1:#{@port}"
@stopped = false
end

Expand Down Expand Up @@ -108,6 +108,52 @@ def test_after_reply
assert_equal true, closed
end

def test_after_reply_exception
@server.app = lambda do |env|
env['rack.after_reply'] << lambda { raise ArgumentError, "oops" }
@simple.call(env)
end

@server.run

socket = TCPSocket.open "127.0.0.1", @port
socket.puts "GET /test HTTP/1.1\r\n"
socket.puts "Connection: Keep-Alive\r\n"
socket.puts "\r\n"

headers = socket.readline("\r\n\r\n")
.split("\r\n")
.drop(1)
.map { |line| line.split(/:\s?/) }
.to_h

content_length = headers["Content-Length"].to_i
real_response_body = socket.read(content_length)

assert_equal "Hello", real_response_body

# When after_reply breaks the connection it will write the expected HTTP
# response followed by a second HTTP response: HTTP/1.1 500
#
# This sleeps to give the server time to write the invalid/extra HTTP
# response.
#
# * If we can read from the socket, we know that extra content has been
# written to the connection and assert that it's our erroneous 500
# response.
# * If we would block trying to read from the socket, we can assume that
# the erroneous 500 response wasn't/won't be written.
sleep 0.1
assert_raises IO::WaitReadable do
content = socket.read_nonblock(12)
refute_includes content, "500"
end

socket.close

stop
end

def test_common_logger
log = StringIO.new

Expand Down