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 thread shutdown responses via low level error messages #2203

Merged
merged 3 commits into from Mar 31, 2020

Conversation

zanker-stripe
Copy link
Contributor

Description

Please describe your pull request. Thank you for contributing! You're the best.

This is a follow on from the suggestions in #2182 (comment). I did not make the change around DefaultLowlevelErrorHandler, because:

puma/lib/puma/server.rb

Lines 819 to 823 in 0b737cc

if @leak_stack_on_error
[500, {}, ["Puma caught this error: #{e.message} (#{e.class})\n#{e.backtrace.join("\n")}"]]
else
[500, {}, ["An unhandled lowlevel error occurred. The application logs may have details.\n"]]
end

The leak_stack_on_error option is stored on the server instance and not options, which leaves no way of getting that data into the handler. I wasn't a fan of passing that through, and reorganizing how leak_stack_on_error works also felt out of scope.

Functionality wise, there's no changes besides thread shutdown now goes through lowlevel error handler and we pass status to the handler based on arity. If leak_stack_on_error is set, we include the stack, if not we show the generic error messages.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added 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.

status = 503
headers = {}
res_body = ["Request was internally terminated early\n"]

rescue Exception => e
@events.unknown_error self, e, "Rack app", env
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this means we pass env on shutdown errors, but that seemed like more of an oversight than an initial design. Happy to unwind if I'm wrong though.

lib/puma/server.rb Outdated Show resolved Hide resolved
lib/puma/server.rb Show resolved Hide resolved
@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Mar 30, 2020
test/test_puma_server.rb Outdated Show resolved Hide resolved
test/test_puma_server.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

1 more thing then this is g2g for me.

@zanker-stripe
Copy link
Contributor Author

Thanks! Changes made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants