Handle thread shutdown responses via low level error messages#2203
Merged
nateberkopec merged 3 commits intopuma:masterfrom Mar 31, 2020
Merged
Handle thread shutdown responses via low level error messages#2203nateberkopec merged 3 commits intopuma:masterfrom
nateberkopec merged 3 commits intopuma:masterfrom
Conversation
ec06a2e to
1c9d4a2
Compare
zanker-stripe
commented
Mar 26, 2020
| res_body = ["Request was internally terminated early\n"] | ||
|
|
||
| rescue Exception => e | ||
| @events.unknown_error self, e, "Rack app", env |
Contributor
Author
There was a problem hiding this comment.
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.
1c9d4a2 to
f0fe6c3
Compare
nateberkopec
requested changes
Mar 30, 2020
8 tasks
nateberkopec
requested changes
Mar 30, 2020
Member
|
1 more thing then this is g2g for me. |
948f17e to
0da294f
Compare
Contributor
Author
|
Thanks! Changes made. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
The
leak_stack_on_erroroption 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 howleak_stack_on_errorworks also felt out of scope.Functionality wise, there's no changes besides thread shutdown now goes through lowlevel error handler and we pass
statusto the handler based on arity. Ifleak_stack_on_erroris set, we include the stack, if not we show the generic error messages.Your checklist for this pull request
[changelog skip]the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.