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.

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

Jump to bottom

Handle fatal error that has no backtrace #2607

Merged
merged 6 commits into from Apr 23, 2021
Merged

Handle fatal error that has no backtrace #2607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d5990a
handle low level error that has no backtrace
calvinxiao Apr 21, 2021
1dad096
add force_shutdown_after in test case
calvinxiao Apr 21, 2021
99d3fcb
skip test on windows for low level error
calvinxiao Apr 21, 2021
f1de8c0
remove extra space
calvinxiao Apr 21, 2021
c2a8067
remove two extra lines
calvinxiao Apr 21, 2021
0428303
rename test method to lowlevel_error
calvinxiao Apr 21, 2021
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
3 changes: 2 additions & 1 deletion lib/puma/server.rb
View file
Open in desktop
Expand Up @@ -528,7 +528,8 @@ def lowlevel_error(e, env, status=500)
end

if @leak_stack_on_error
[status, {}, ["Puma caught this error: #{e.message} (#{e.class})\n#{e.backtrace.join("\n")}"]]
backtrace = e.backtrace.nil? ? '<no backtrace available>' : e.backtrace.join("\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nitpick: in this case, the ternary doesn't make a lot of sense. Say e.backtrace is false (i mean, it can't be, but say that it is): then we're going to call false.join("\n"). What you really care about is the ability to respond_to? the join method, so you should check for that instead.

Copy link
Member

@MSP-Greg MSP-Greg Apr 23, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current Ruby docs for Exception#backtrace state:

'In the case no backtrace has been set, nil is returned'

That seems imply that the method always exists? We can't join nil...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's why I said it's a style nitpick and not a correctness nitpick 😄 IMO anything passing the test in the ternary should work

Copy link
Member

@MSP-Greg MSP-Greg Apr 23, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could check whether the backtrace returns an array?

Sometimes with stuff like this it may better to keep the scope to what Ruby docs state, so if the Ruby API changes, we might see it in tests? Believe it or not, I've seen things like that happen working with Ruby master and even OpenSSL. IOW, keep the test very narrow, and edges cases appear.

JFYI, I have no style... And, thanks for reviewing/merging.

[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
Expand Down
19 changes: 19 additions & 0 deletions test/test_puma_server.rb
View file
Open in desktop
Expand Up @@ -285,6 +285,25 @@ def test_force_shutdown_custom_error_message
assert_match(/{}\n$/, data)
end

def test_lowlevel_error_message
skip_if :windows
@server = Puma::Server.new @app, @events, {:force_shutdown_after => 2}

server_run app: ->(env) do
require 'json'

# will raise fatal: machine stack overflow in critical region
obj = {}
obj['cycle'] = obj
::JSON.dump(obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a reliable test for this issue because:

  • Not all Ruby implementations might expose no backtrace for a stack overflow (that's pretty unhelpful, isn't it?)
  • a stack overflow is a regular exception but given it messes the VM badly you can consider the VM corrupted after it, so maybe stack overflows should become fatal in the future, since it's very rarely safe to rescue it (you would need no ensure and no finally in the VM, and no second stack overflow while handling the stack overflow very close to the limit, and probably other things which cannot be guaranteed)
  • More details on Use the bundler shipped with ruby for non-MRI #2811

Maybe we can just test creating an exception and using set_backtrace(nil)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 @eregon I created #2813 with your comments for visibility so anyone can pick this up

end

data = send_http_and_read "GET / HTTP/1.0\r\n\r\n"

assert_match(/HTTP\/1.0 500 Internal Server Error/, data)
assert (data.size > 0), "Expected response message to be not empty"
end

def test_force_shutdown_error_default
@server = Puma::Server.new @app, @events, {:force_shutdown_after => 2}

Expand Down