Always send lowlevel_error response to client #2731

baelter · 2021-10-27T14:09:03Z

Description

Always send what the lowlevel_error handler returns to the client.

Refactors some code out of Request#handle_request to a new Request#write_response that can be used from Server to send the response from lowlevel_error
Closes #2341

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

test/test_puma_server.rb

dentarg · 2021-10-28T07:15:34Z

test/test_response_header.rb

@@ -45,15 +45,15 @@ 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.1 500 Internal Server Error/, data)
+    assert_match(/HTTP\/1.0 500 Internal Server Error/, data)


Why did this have to change? Looks like something isn't working now? This is the failure without this change:

Errors & Failures: 1) Failure: TestResponseHeader#test_integer_key [test/test_response_header.rb:48]: Expected /HTTP\/1.1 500 Internal Server Error/ to match # encoding: ASCII-8BIT # valid: true "HTTP/1.0 200 OK\r\nHTTP/1.0 500 Internal Server Error\r\nContent-Length: 559\r\n\r\nPuma caught this error: undefined method `downcase' for 1:Integer (NoMethodError)\n/Users/dentarg/src/puma/lib/puma/request.rb:437:in `block in str_headers'\n/Users/dentarg/src/puma/lib/puma/request.rb:434:in `each'\n/Users/dentarg/src/puma/lib/puma/request.rb:434:in `str_headers'\n/Users/dentarg/src/puma/lib/puma/request.rb:122:in `write_response'\n/Users/dentarg/src/puma/lib/puma/request.rb:97:in `handle_request'\n/Users/dentarg/src/puma/lib/puma/server.rb:447:in `process_client'\n/Users/dentarg/src/puma/lib/puma/thread_pool.rb:147:in `block in spawn_thread'".

If I make the assert fail on master it looks like

Run options: --verbose --name /test_integer_key/ --seed 19438 # Running: TestResponseHeader#test_integer_key = 0.02 s = F Fabulous run in 0.024515s, 40.7914 runs/s, 81.5827 assertions/s. Errors & Failures: 1) Failure: TestResponseHeader#test_integer_key [test/test_response_header.rb:48]: Expected /aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/ to match # encoding: ASCII-8BIT # valid: true "HTTP/1.1 500 Internal Server Error\r\n\r\n". 1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

I guess it is coming from this code and the fact that @leak_stack_on_error is true in test and development

puma/lib/puma/server.rb

Lines 543 to 548 in 7812f1b

if @leak_stack_on_error

backtrace = e.backtrace.nil? ? '<no backtrace available>' : e.backtrace.join("\n")

[status, {}, ["Puma caught this error: #{e.message} (#{e.class})\n#{backtrace}"]]

else

[status, {}, ["An unhandled lowlevel error occurred. The application logs may have details.\n"]]

end

Maybe it has "always" been like this... but the start with HTTP/1.0 200 OK\r\n looks strange?

I'm not entirely sure, but the spec send a HTTP 1.0 request so I think it makes sense it gets a HTTP 1.0 response back. Maybe something was off before :) ?

Yeah, it was hard coded before: https://github.com/baelter/puma/blob/a2bcda414377ee3f5855a66ed83aa41ce6f0a29d/lib/puma/const.rb#L139

But now we use the correct version.

There's been a change though, this tests triggers an exception in str_headers and if we look at str_headers it has already populated the buffer with a 200 response, that is also included when we try write the error response with write_response.

Ah, nice catch

nateberkopec · 2021-11-01T21:27:27Z

Thank you for trying to close such a long-standing issue!

test/test_puma_server.rb

cjlarose

Awesome work!

cjlarose · 2021-11-02T18:41:43Z

Changes look good to me. If it looks good to you guys @nateberkopec and @dentarg , I think we're good to merge.

dentarg · 2021-11-02T19:27:44Z

I should disclose that @baelter is my co-worker at 84codes, hence I was quick to look at this one. I think a second approve from someone else would be great, maybe @MSP-Greg if @nateberkopec doesn't have the time right now?

baelter · 2021-11-03T07:56:48Z

Awesome work!

Thanks!

This reverts commit f8acac1. In response to puma#2808

This reverts commit f8acac1. In response to #2808

)" (puma#2809)" This reverts commit 7008a61.

* Always send lowlevel_error response to client * Add spec for lowlever error handler * Make sure we have a clean buffer when starting response * Simplify test * Rename spec * Add method docs * Tweak the test * Return 500 from lowlevel_error_handler in test Co-authored-by: Patrik Ragnarsson <patrik@starkast.net>

…uma#2809) This reverts commit f8acac1. In response to puma#2808

baelter marked this pull request as ready for review October 28, 2021 06:29

dentarg reviewed Oct 28, 2021

View reviewed changes

test/test_puma_server.rb Outdated Show resolved Hide resolved

dentarg reviewed Oct 28, 2021

View reviewed changes

dentarg requested a review from cjlarose October 28, 2021 10:15

cjlarose self-assigned this Oct 28, 2021

dentarg force-pushed the always-return-lowlevel-errors branch from 228d4a3 to 388141a Compare October 29, 2021 14:32

MSP-Greg added the feature label Oct 29, 2021

baelter and others added 7 commits October 31, 2021 14:42

Always send lowlevel_error response to client

9e8eaac

Add spec for lowlever error handler

a5d83c3

Make sure we have a clean buffer when starting response

def464a

Simplify test

7d53fef

Rename spec

9a96822

Add method docs

9020b01

Tweak the test

40c72f9

dentarg force-pushed the always-return-lowlevel-errors branch from 388141a to 40c72f9 Compare October 31, 2021 13:42

dentarg added the waiting-for-review Waiting on review from anyone label Oct 31, 2021

nateberkopec reviewed Nov 1, 2021

View reviewed changes

test/test_puma_server.rb Outdated Show resolved Hide resolved

Return 500 from lowlevel_error_handler in test

3bb0ace

cjlarose removed their assignment Nov 2, 2021

cjlarose approved these changes Nov 2, 2021

View reviewed changes

MSP-Greg approved these changes Nov 2, 2021

View reviewed changes

MSP-Greg merged commit f8acac1 into puma:master Nov 2, 2021

MSP-Greg mentioned this pull request Jan 26, 2022

Authentication with cookies unreliable on 5.6.0 #2808

Closed

dentarg added a commit to dentarg/puma that referenced this pull request Jan 26, 2022

Revert "Always send lowlevel_error response to client (puma#2731)"

eb1f1e5

This reverts commit f8acac1. In response to puma#2808

nateberkopec pushed a commit that referenced this pull request Jan 26, 2022

Revert "Always send lowlevel_error response to client (#2731)" (#2809)

7008a61

This reverts commit f8acac1. In response to #2808

baelter added a commit to baelter/puma that referenced this pull request Jan 27, 2022

Revert "Revert "Always send lowlevel_error response to client (puma#2731

cfde8a7

)" (puma#2809)" This reverts commit 7008a61.

nateberkopec mentioned this pull request Jan 28, 2022

Always send lowlevel_error response to client #2812

Closed

7 tasks

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022

Revert "Always send lowlevel_error response to client (puma#2731)" (p…

d8bca72

…uma#2809) This reverts commit f8acac1. In response to puma#2808

dentarg mentioned this pull request Dec 15, 2022

Puma sends empty response bodies even when low-level handler is specified #2341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always send lowlevel_error response to client #2731

Always send lowlevel_error response to client #2731

baelter commented Oct 27, 2021 •

edited

dentarg Oct 28, 2021

dentarg Oct 28, 2021

baelter Oct 28, 2021

baelter Oct 28, 2021

dentarg Oct 28, 2021

baelter Oct 28, 2021

nateberkopec commented Nov 1, 2021

cjlarose left a comment

cjlarose commented Nov 2, 2021

dentarg commented Nov 2, 2021

baelter commented Nov 3, 2021

	if @leak_stack_on_error
	backtrace = e.backtrace.nil? ? '<no backtrace available>' : e.backtrace.join("\n")
	[status, {}, ["Puma caught this error: #{e.message} (#{e.class})\n#{backtrace}"]]
	else
	[status, {}, ["An unhandled lowlevel error occurred. The application logs may have details.\n"]]
	end

Always send lowlevel_error response to client #2731

Always send lowlevel_error response to client #2731

Conversation

baelter commented Oct 27, 2021 • edited

Description

Your checklist for this pull request

dentarg Oct 28, 2021

Choose a reason for hiding this comment

dentarg Oct 28, 2021

Choose a reason for hiding this comment

baelter Oct 28, 2021

Choose a reason for hiding this comment

baelter Oct 28, 2021

Choose a reason for hiding this comment

dentarg Oct 28, 2021

Choose a reason for hiding this comment

baelter Oct 28, 2021

Choose a reason for hiding this comment

nateberkopec commented Nov 1, 2021

cjlarose left a comment

Choose a reason for hiding this comment

cjlarose commented Nov 2, 2021

dentarg commented Nov 2, 2021

baelter commented Nov 3, 2021

baelter commented Oct 27, 2021 •

edited