rubygems
/
puma
1
0
Fork 0
Code Packages Releases Activity

Correctly close app body for all code paths (#3002) 

* request.rb - Close app body always

* test_rack_server.rb - add test_hijack_body_close

* puma_server.rb - refactor, takes too long

* Add `test_lowlevel_error_body_close` to verify body close call when lowlevel_error

* [CI] Fix two intermittent failures?
This commit is contained in:
MSP-Greg 2022-11-03 01:46:43 -05:00 committed by GitHub
parent 9e131b6098
commit 3d33475dd4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 118 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
lib/puma/request.rb
View File

@ -48,6 +48,7 @@ module Puma
def handle_request(client, io_buffer, requests)
env = client.env
socket = client.io # io may be a MiniSSL::Socket
app_body = nil
return false if closed_socket?(socket)
@ -85,14 +86,18 @@ module Puma
begin
if SUPPORTED_HTTP_METHODS.include?(env[REQUEST_METHOD])
status, headers, res_body = @thread_pool.with_force_shutdown do
status, headers, app_body = @thread_pool.with_force_shutdown do
@app.call(env)
end
else
@log_writer.log "Unsupported HTTP method used: #{env[REQUEST_METHOD]}"
status, headers, res_body = [501, {}, ["#{env[REQUEST_METHOD]} method is not supported"]]
status, headers, app_body = [501, {}, ["#{env[REQUEST_METHOD]} method is not supported"]]
end
# app_body needs to always be closed, hold value in case lowlevel_error
# is called
res_body = app_body
return :async if client.hijacked
status = status.to_i
@ -115,6 +120,17 @@ module Puma
status, headers, res_body = lowlevel_error(e, env, 500)
end
prepare_response(status, headers, res_body, io_buffer, requests, client)
ensure
io_buffer.reset
uncork_socket client.io
app_body.close if app_body.respond_to? :close
client.tempfile&.unlink
after_reply = env[RACK_AFTER_REPLY] || []
begin
after_reply.each { |o| o.call }
rescue StandardError => e
@log_writer.debug_error e
end unless after_reply.empty?
end
# Assembles the headers and prepares the body for actually sending the
@ -122,52 +138,50 @@ module Puma
#
# @param status [Integer] the status returned by the Rack application
# @param headers [Hash] the headers returned by the Rack application
# @param app_body [Array] the body returned by the Rack application or
# @param res_body [Array] the body returned by the Rack application or
# a call to `lowlevel_error`
# @param io_buffer [Puma::IOBuffer] modified in place
# @param requests [Integer] number of inline requests handled
# @param client [Puma::Client]
# @return [Boolean,:async]
def prepare_response(status, headers, app_body, io_buffer, requests, client)
def prepare_response(status, headers, res_body, io_buffer, requests, client)
env = client.env
socket = client.io
after_reply = env[RACK_AFTER_REPLY] || []
socket = client.io
return false if closed_socket?(socket)
resp_info = str_headers(env, status, headers, app_body, io_buffer, requests, client)
resp_info = str_headers(env, status, headers, res_body, io_buffer, requests, client)
# below converts app_body into body, dependent on app_body's characteristics, and
# resp_info[:content_length] will be set if it can be determined
if !resp_info[:content_length] && !resp_info[:transfer_encoding] && status != 204
if app_body.respond_to?(:to_ary)
if res_body.respond_to?(:to_ary)
length = 0
if array_body = app_body.to_ary
if array_body = res_body.to_ary
body = array_body.map { |part| length += part.bytesize; part }
elsif app_body.is_a?(::File) && app_body.respond_to?(:size)
length = app_body.size
elsif app_body.respond_to?(:each)
elsif res_body.is_a?(::File) && res_body.respond_to?(:size)
length = res_body.size
elsif res_body.respond_to?(:each)
body = []
app_body.each { |part| length += part.bytesize; body << part }
res_body.each { |part| length += part.bytesize; body << part }
end
resp_info[:content_length] = length
elsif app_body.is_a?(File) && app_body.respond_to?(:size)
resp_info[:content_length] = app_body.size
body = app_body
elsif app_body.respond_to?(:to_path) && app_body.respond_to?(:each) &&
File.readable?(fn = app_body.to_path)
elsif res_body.is_a?(File) && res_body.respond_to?(:size)
resp_info[:content_length] = res_body.size
body = res_body
elsif res_body.respond_to?(:to_path) && res_body.respond_to?(:each) &&
File.readable?(fn = res_body.to_path)
body = File.open fn, 'rb'
resp_info[:content_length] = body.size
else
body = app_body
body = res_body
end
elsif !app_body.is_a?(::File) && app_body.respond_to?(:to_path) && app_body.respond_to?(:each) &&
File.readable?(fn = app_body.to_path)
elsif !res_body.is_a?(::File) && res_body.respond_to?(:to_path) && res_body.respond_to?(:each) &&
File.readable?(fn = res_body.to_path)
body = File.open fn, 'rb'
resp_info[:content_length] = body.size
else
body = app_body
body = res_body
end
line_ending = LINE_END
@ -175,8 +189,8 @@ module Puma
content_length = resp_info[:content_length]
keep_alive = resp_info[:keep_alive]
if app_body && !app_body.respond_to?(:each)
response_hijack = app_body
if res_body && !res_body.respond_to?(:each)
response_hijack = res_body
else
response_hijack = resp_info[:response_hijack]
end
@ -210,18 +224,6 @@ module Puma
fast_write_response socket, body, io_buffer, chunked, content_length.to_i
keep_alive
ensure
io_buffer.reset
resp_info = nil
uncork_socket socket
app_body.close if app_body.respond_to? :close
client.tempfile&.unlink
begin
after_reply.each { |o| o.call }
rescue StandardError => e
@log_writer.debug_error e
end unless after_reply.empty?
end
# @param env [Hash] see Puma::Client#env, from request

55
test/test_puma_server.rb
View File

@ -59,6 +59,11 @@ class TestPumaServer < Minitest::Test
header
end
# only for shorter bodies!
def send_http_and_sysread(req)
send_http(req).sysread 2_048
end
def send_http_and_read(req)
send_http(req).read
end
@ -419,30 +424,54 @@ EOF
assert_match(/{}\n$/, data)
end
def test_lowlevel_error_message
@server = Puma::Server.new @app, @events, {log_writer: @log_writer, :force_shutdown_after => 2}
class ArrayClose < Array
attr_reader :is_closed
def closed?
@is_closed
end
server_run do
def close
@is_closed = true
end
end
# returns status as an array, which throws lowlevel error
def test_lowlevel_error_body_close
app_body = ArrayClose.new(['lowlevel_error'])
server_run(log_writer: @log_writer, :force_shutdown_after => 2) do
[[0,1], {}, app_body]
end
data = send_http_and_sysread "GET / HTTP/1.0\r\n\r\n"
assert_includes data, 'HTTP/1.0 500 Internal Server Error'
assert_includes data, "Puma caught this error: undefined method `to_i' for [0, 1]:Array"
refute_includes data, 'lowlevel_error'
sleep 0.1 unless ::Puma::IS_MRI
assert app_body.closed?
end
def test_lowlevel_error_message
server_run(log_writer: @log_writer, :force_shutdown_after => 2) do
raise NoMethodError, "Oh no an error"
end
data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"
data = send_http_and_sysread "GET / HTTP/1.0\r\n\r\n"
assert_match(/HTTP\/1.0 500 Internal Server Error/, data)
assert_includes data, 'HTTP/1.0 500 Internal Server Error'
assert_match(/Puma caught this error: Oh no an error.*\(NoMethodError\).*test\/test_puma_server.rb/m, data)
end
def test_lowlevel_error_message_without_backtrace
@server = Puma::Server.new @app, @events, {log_writer: @log_writer, :force_shutdown_after => 2}
server_run do
server_run(log_writer: @log_writer, :force_shutdown_after => 2) do
raise WithoutBacktraceError.new
end
data = send_http_and_read "GET / HTTP/1.1\r\n\r\n"
assert_match(/HTTP\/1.1 500 Internal Server Error/, data)
assert_match(/Puma caught this error: no backtrace error.*\(WithoutBacktraceError\)/, data)
assert_match(/<no backtrace available>/, data)
data = send_http_and_sysread "GET / HTTP/1.1\r\n\r\n"
assert_includes data, 'HTTP/1.1 500 Internal Server Error'
assert_includes data, 'Puma caught this error: no backtrace error (WithoutBacktraceError)'
assert_includes data, '<no backtrace available>'
end
def test_force_shutdown_error_default
@ -1439,7 +1468,7 @@ EOF
# TODO: it would be great to test a connection from a non-localhost IP, but we can't really do that. For
# now, at least test that it doesn't return garbage.
remote_addr = send_http_and_read("GET / HTTP/1.1\r\n\r\n").split("\r\n").last
remote_addr = send_http_and_sysread("GET / HTTP/1.1\r\n\r\n").split("\r\n").last
assert_equal @host, remote_addr
end
end

38
test/test_rack_server.rb
View File

@ -210,6 +210,43 @@ class TestRackServer < Minitest::Test
assert_equal str_ary_bytes, content_length
end
def test_hijack_body_close
available = true
@server.app = ->(env) {
if available
available = false
hijack_lambda = ->(io) {
io.syswrite 'hijacked'
io.close
}
[200, { 'Content-Type' => 'text/plain', 'rack.hijack' => hijack_lambda},
::Rack::BodyProxy.new([]) { available = true }]
else
[500, { 'Content-Type' => 'text/plain' }, ['incorrect']]
end
}
@server.run
socket1 = TCPSocket.new "127.0.0.1", @port
socket1.syswrite "GET / HTTP/1.1\r\n\r\n"
sleep 0.25 if Puma::IS_WINDOWS || !Puma::IS_MRI
resp1 = socket1.sysread 1_024
sleep 0.01 # time for close block to be called ?
socket2 = TCPSocket.new "127.0.0.1", @port
socket2.syswrite "GET / HTTP/1.1\r\n\r\n"
sleep 0.25 if Puma::IS_WINDOWS || !Puma::IS_MRI
resp2 = socket2.sysread 1_024
assert_operator resp1, :end_with?, 'hijacked'
assert_operator resp2, :end_with?, 'hijacked'
socket1.close
socket2.close
end
def test_common_logger
log = StringIO.new
@ -259,5 +296,4 @@ class TestRackServer < Minitest::Test
assert_includes headers.downcase, TRANSFER_ENCODING_CHUNKED
assert_equal STR_1KB * 10, resp_body
end
end