Handle throwing in controller action in log subscriber #41223
Merged
Conversation
This file contains 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
janko
force-pushed
the
controller-throw-log-subscriber
branch
2 times, most recently
from
21868b9
to
da34a83
Compare
kamipo
reviewed
Jan 24, 2021
When throw was used in a controller action, and there is matching catch
around the request in a Rack middleware, then :exception won't be
present in the event payload.
This is because ActiveSupport::Notifications::Instrumenter.instrument
sets :exception in a rescue handler, but rescue is never called in a
throw/catch scenario:
catch(:halt) do
begin
throw :halt
rescue Exception => e
puts "rescue" # never reached
ensure
puts "ensure"
end
end
Missing :exception was actually handled prior to Rails 6.1.0, but an
optimization updated the code to assume this was present. So this can be
considered a regression fix.
janko
force-pushed
the
controller-throw-log-subscriber
branch
from
da34a83
to
53adf53
Compare
|
The test failures are already fixed in current main branch #41203. |
kamipo
added a commit
that referenced
this pull request
Jan 24, 2021
Handle throwing in controller action in log subscriber
This was referenced Mar 12, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
When throw was used in a controller action, and there is matching catch around the request in a Rack middleware, then
:exceptionwon't be present in the event payload.This is because
ActiveSupport::Notifications::Instrumenter.instrumentsets:exceptionin a rescue handler, but rescue is never called in a throw/catch scenario:Missing
:exceptionwas actually handled prior to Rails 6.1.0, but an optimization updated the code to assume:exceptionwas present. I found it easiest to just revert the commit. This PR can therefore be considered a regression fix.Other Information
I've experienced this issue when using Rodauth with Rails, which throws
:haltwhen redirecting (with Rodauth middleware catching:halt), and there are use cases in which Rodauth's redirect method is called inside a controller action.