From 7008a61ac9f87f71f4c688a6ab5d3cf2b6c8151a Mon Sep 17 00:00:00 2001 From: Patrik Ragnarsson Date: Thu, 27 Jan 2022 00:45:54 +0100 Subject: [PATCH] Revert "Always send lowlevel_error response to client (#2731)" (#2809) This reverts commit f8acac1f0702fea1a4f88d68a40bb2f53650b14c. In response to https://github.com/puma/puma/issues/2808 --- lib/puma/request.rb | 76 ++++++++++++++---------------------- lib/puma/server.rb | 20 +++++----- test/test_puma_server.rb | 11 ------ test/test_response_header.rb | 4 +- 4 files changed, 40 insertions(+), 71 deletions(-) diff --git a/lib/puma/request.rb b/lib/puma/request.rb index 8950cf282a..948992895f 100644 --- a/lib/puma/request.rb +++ b/lib/puma/request.rb @@ -46,7 +46,11 @@ def handle_request(client, lines, requests) env[HIJACK_P] = true env[HIJACK] = client - env[RACK_INPUT] = client.body + body = client.body + + head = env[REQUEST_METHOD] == HEAD + + env[RACK_INPUT] = body env[RACK_URL_SCHEME] ||= default_server_port(env) == PORT_443 ? HTTPS : HTTP if @early_hints @@ -65,58 +69,36 @@ def handle_request(client, lines, requests) # A rack extension. If the app writes #call'ables to this # array, we will invoke them when the request is done. # - env[RACK_AFTER_REPLY] = [] + after_reply = env[RACK_AFTER_REPLY] = [] begin - status, headers, res_body = @thread_pool.with_force_shutdown do - @app.call(env) - end - - return :async if client.hijacked - - status = status.to_i - - if status == -1 - unless headers.empty? and res_body == [] - raise "async response must have empty headers and body" + begin + status, headers, res_body = @thread_pool.with_force_shutdown do + @app.call(env) end - return :async - end - rescue ThreadPool::ForceShutdown => e - @events.unknown_error e, client, "Rack app" - @events.log "Detected force shutdown of a thread" - - status, headers, res_body = lowlevel_error(e, env, 503) - rescue Exception => e - @events.unknown_error e, client, "Rack app" + return :async if client.hijacked - status, headers, res_body = lowlevel_error(e, env, 500) - end + status = status.to_i - write_response(status, headers, res_body, lines, requests, client) - end + if status == -1 + unless headers.empty? and res_body == [] + raise "async response must have empty headers and body" + end - # Does the actual response writing for Request#handle_request and Server#client_error - # - # @param status [Integer] the status returned by the Rack application - # @param headers [Hash] the headers returned by the Rack application - # @param res_body [Array] the body returned by the Rack application - # @param lines [Puma::IOBuffer] modified in place - # @param requests [Integer] number of inline requests handled - # @param client [Puma::Client] - # @return [Boolean,:async] - def write_response(status, headers, res_body, lines, requests, client) - env = client.env - io = client.io + return :async + end + rescue ThreadPool::ForceShutdown => e + @events.unknown_error e, client, "Rack app" + @events.log "Detected force shutdown of a thread" - return false if closed_socket?(io) - lines.clear + status, headers, res_body = lowlevel_error(e, env, 503) + rescue Exception => e + @events.unknown_error e, client, "Rack app" - head = env[REQUEST_METHOD] == HEAD - after_reply = env[RACK_AFTER_REPLY] || [] + status, headers, res_body = lowlevel_error(e, env, 500) + end - begin res_info = {} res_info[:content_length] = nil res_info[:no_body] = head @@ -167,9 +149,9 @@ def write_response(status, headers, res_body, lines, requests, client) res_body.each do |part| next if part.bytesize.zero? if chunked - fast_write io, (part.bytesize.to_s(16) << line_ending) - fast_write io, part # part may have different encoding - fast_write io, line_ending + fast_write io, (part.bytesize.to_s(16) << line_ending) + fast_write io, part # part may have different encoding + fast_write io, line_ending else fast_write io, part end @@ -187,7 +169,7 @@ def write_response(status, headers, res_body, lines, requests, client) ensure uncork_socket io - client.body.close if client.body + body.close client.tempfile.unlink if client.tempfile res_body.close if res_body.respond_to? :close diff --git a/lib/puma/server.rb b/lib/puma/server.rb index 88cee7cefe..f4aed5dfc5 100644 --- a/lib/puma/server.rb +++ b/lib/puma/server.rb @@ -476,7 +476,7 @@ def process_client(client, buffer) end true rescue StandardError => e - client_error(e, client, buffer, requests) + client_error(e, client) # The ensure tries to close +client+ down requests > 0 ensure @@ -504,36 +504,34 @@ def with_force_shutdown(client, &block) # :nocov: # Handle various error types thrown by Client I/O operations. - def client_error(e, client, buffer = ::Puma::IOBuffer.new, requests = 1) + def client_error(e, client) # Swallow, do not log return if [ConnectionError, EOFError].include?(e.class) + lowlevel_error(e, client.env) case e when MiniSSL::SSLError @events.ssl_error e, client.io when HttpParserError - status, headers, res_body = lowlevel_error(e, client.env, 400) - write_response(status, headers, res_body, buffer, requests, client) + client.write_error(400) @events.parse_error e, client else - status, headers, res_body = lowlevel_error(e, client.env) - write_response(status, headers, res_body, buffer, requests, client) + client.write_error(500) @events.unknown_error e, nil, "Read" end end # A fallback rack response if +@app+ raises as exception. # - def lowlevel_error(e, env, status = 500) + def lowlevel_error(e, env, status=500) if handler = @options[:lowlevel_error_handler] if handler.arity == 1 - handler_status, headers, res_body = handler.call(e) + return handler.call(e) elsif handler.arity == 2 - handler_status, headers, res_body = handler.call(e, env) + return handler.call(e, env) else - handler_status, headers, res_body = handler.call(e, env, status) + return handler.call(e, env, status) end - return [handler_status || status, headers || {}, res_body || []] end if @leak_stack_on_error diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index c70d8aa97e..bfa2b977e3 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -363,17 +363,6 @@ def test_lowlevel_error_message assert (data.size > 0), "Expected response message to be not empty" end - def test_lowlevel_error_handler_custom_response - options = { lowlevel_error_handler: ->(_err) { [500, {}, ["error page"]] } } - # setting the headers argument to nil will trigger exception inside Puma - broken_app = ->(_env) { [200, nil, []] } - server_run(**options, &broken_app) - - data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" - - assert_match %r{HTTP/1.0 500 Internal Server Error\r\nContent-Length: 10\r\n\r\nerror page}, data - end - def test_force_shutdown_error_default server_run(force_shutdown_after: 2) do @server.stop diff --git a/test/test_response_header.rb b/test/test_response_header.rb index 03fb43ca54..508bbcd78c 100644 --- a/test/test_response_header.rb +++ b/test/test_response_header.rb @@ -45,7 +45,7 @@ def test_integer_key server_run app: ->(env) { [200, { 1 => 'Boo'}, []] } data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" - assert_match(/HTTP\/1.0 500 Internal Server Error/, data) + assert_match(/HTTP\/1.1 500 Internal Server Error/, data) end # The header must respond to each @@ -53,7 +53,7 @@ def test_nil_header server_run app: ->(env) { [200, nil, []] } data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" - assert_match(/HTTP\/1.0 500 Internal Server Error/, data) + assert_match(/HTTP\/1.1 500 Internal Server Error/, data) end # The values of the header must be Strings