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

Handle fatal error that has no backtrace #2607

Merged
merged 6 commits into from Apr 23, 2021

Conversation

calvinxiao
Copy link
Contributor

@calvinxiao calvinxiao commented Apr 21, 2021

Description

Handle error that e.backtrace is nil.

Current implementation assumes e.backtrace is an Array, and will raise NoMethodError to the top of thread-pool, this will output an empty response body.

Closes #2552

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.

@calvinxiao
Copy link
Contributor Author

@MSP-Greg

  1. I added test in test_puma_server.rb, is it appropriate?
  2. This might need backport.

@MSP-Greg
Copy link
Member

I added test in test_puma_server.rb, is it appropriate?

Yes.

Maybe remove the two extra lines after the test?

@calvinxiao
Copy link
Contributor Author

I added test in test_puma_server.rb, is it appropriate?

Yes.

Maybe remove the two extra lines after the test?

Good catch, pushed a new commit.

@calvinxiao calvinxiao marked this pull request as ready for review April 21, 2021 03:32
@nateberkopec
Copy link
Member

@calvinxiao Backports are optional on Puma. Mostly we just don't have the time to do them. We do them for security issues but for bugfixes, no one has taken up the job of "backport person".

That's 100% something you or someone else can do.

@@ -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

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

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.

@nateberkopec
Copy link
Member

nateberkopec commented Apr 23, 2021

Why is the test marked as skip on Windows?

@MSP-Greg
Copy link
Member

MSP-Greg commented Apr 23, 2021

Why is the test marked as skip on Windows?

Appears to be a rather odd Windows bug, not sure about other OS's...

EDIT: I should have said 'Windows Ruby bug'

# 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

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* handle low level error that has no backtrace

* add force_shutdown_after in test case

* skip test on windows for low level error

* remove extra space

* remove two extra lines

* rename test method to lowlevel_error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Child workers becoming zombies/unreachable after exception
5 participants