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

zanker-stripe commented Mar 26, 2020

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.
@zanker-stripe zanker-stripe force-pushed the zanker:zanker/lowlevel-errors-threads branch from ec06a2e to 1c9d4a2 Mar 26, 2020
status = 503
headers = {}
res_body = ["Request was internally terminated early\n"]

rescue Exception => e
@events.unknown_error self, e, "Rack app", env

This comment has been minimized.

Copy link
@zanker-stripe

zanker-stripe Mar 26, 2020

Author Contributor

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.

@zanker-stripe zanker-stripe force-pushed the zanker:zanker/lowlevel-errors-threads branch from 1c9d4a2 to f0fe6c3 Mar 26, 2020
lib/puma/server.rb Outdated Show resolved Hide resolved
lib/puma/server.rb Show resolved Hide resolved
test/test_puma_server.rb Outdated Show resolved Hide resolved
test/test_puma_server.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

nateberkopec commented Mar 30, 2020

1 more thing then this is g2g for me.

@zanker-stripe zanker-stripe force-pushed the zanker:zanker/lowlevel-errors-threads branch from 948f17e to 0da294f Mar 31, 2020
@zanker-stripe zanker-stripe requested a review from nateberkopec Mar 31, 2020
@zanker-stripe
Copy link
Contributor Author

zanker-stripe commented Mar 31, 2020

Thanks! Changes made.

@nateberkopec nateberkopec merged commit dec41f6 into puma:master Mar 31, 2020
27 checks passed
27 checks passed
build
Details
ubuntu-18.04, 2.2
Details
ubuntu-18.04, 2.3
Details
ubuntu-18.04, 2.4
Details
ubuntu-18.04, 2.5
Details
ubuntu-18.04, 2.6
Details
ubuntu-18.04, 2.7
Details
ubuntu-18.04, head
Details
ubuntu-18.04, jruby
Details
ubuntu-18.04, truffleruby-head
Details
macos, 2.2
Details
macos, 2.3
Details
macos, 2.4
Details
macos, 2.5
Details
macos, 2.6
Details
macos, 2.7
Details
macos, head
Details
macos, jruby
Details
macos, truffleruby-head
Details
windows-latest, 2.2
Details
windows-latest, 2.3
Details
windows-latest, 2.4
Details
windows-latest, 2.5
Details
windows-latest, 2.6
Details
windows-latest, 2.7
Details
windows-latest, mingw
Details
ubuntu-latest, jruby-head ubuntu-latest, jruby-head
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.