Guard against subscriber exceptions in ActiveSupport::Notifications#43282
Conversation
dd862cc to
8afcbb6
Compare
|
One open question here is whether to adjust the default implementation to be safer as is done in this PR (but requires changes to tests that rely on this behaviour, which may mean it's too expected), or whether to use an optional subclassed That said, I do think the consistency issues with the unfortunate usage of |
b17cc36 to
f4a7942
Compare
|
|
||
| listeners.each do |s| | ||
| yield s | ||
| rescue => e |
There was a problem hiding this comment.
Should not we use rescue Exception => e here? If our goal is make sure no exception will stop the chain I think it makes sense,
There was a problem hiding this comment.
👍 I think that does make sense, given the consistency issues would be equally bad regardless of exception type.
|
Thanks!
If we solve these with an approach like #43241 does that make this less necessary or is there still a need? |
I think there is an argument that it would be nice to see instrumenters work as independently as possible from each-other, regardless of whether the rails internals use a In any case, #43241 still relies on |
Ref: rails#43282 Ref: rails#43561 It can be legitimate for subscriber to want to bubble up some exception to the caller, so wrapping it change the exception class which might break the calling code expecting a specific error. We can minimize this by only using InstrumentationSubscriberError when more than one subscriber raised.
Previously in rails#43282, which shipped with Rails 7.0, we added the guarantee that if an exception occurred within an ActiveSupport::Notificaitons subscriber that the remaining subscribers would still be run. This was broken in rails#44469, where the different types of subscribers were broken into groups by type for performance. Although we would guard exceptions and allways run all (or none) of the same group, that didn't work cross-group with different types of subscriber.
Previously in rails#43282, which shipped with Rails 7.0, we added the guarantee that if an exception occurred within an ActiveSupport::Notificaitons subscriber that the remaining subscribers would still be run. This was broken in rails#44469, where the different types of subscribers were broken into groups by type for performance. Although we would guard exceptions and allways run all (or none) of the same group, that didn't work cross-group with different types of subscriber.
Summary
Currently the fanout loops in
ActiveSupport::Notifications::Fanoutto send notifications to subscribers performs no exception handling, meaning any exceptions in a subscriber immediately break out of the notification loop.This means notifications are delivered to an arbitrary subset of the listeners, depending on subscription order. Additionally, some of these listeners (especially the ones defined in the
Fanoutclass) rely on aThread.currentstack to store information about start events for when finish events are later emitted. If an exception occurs during either the start or finish process in one listener, other listeners will unfortunately be left uncalled, leaving a consistency problem because the stack will still contain the elements that are complete but never had#finishcalled due to an earlier exception.This PR improves this situation by ensuring that fanout events will always run on all listeners, with any exceptions accumulated and then wrapped in an
InstrumentationSubscriberError. This means an erroring#finishsubscriber (the most likely) will cause all working instrumentation to succeed, before aInstrumentationSubscriberErroris finally raised for any broken ones. This ensures that the minimal number of subscribers are impacted, and in the case of theThread.currentstack, also ensures that the stack is cleaned up for all theTimed,MonotonicTimed, etc listener classes defined here.Callers can catch
InstrumentationSubscriberErrorand inspect theexceptionsattribute to find the original exceptions from subscribers and handle them accordingly.This PR touches the same code paths as #43241, and will be trivial to adjust for that PR by specifying the iteration type like
iterate_guarding_exceptions(..., :each)anditerate_guarding_exceptions(..., :map), but I left this additional complexity out of this PR so it was simpler, and can update the other PR as needed if this one is accepted.