Skip to content

Commit 7beaacc

Browse files
committed
Fix exception guards on multiple subscriber types
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.
1 parent a475175 commit 7beaacc

File tree

2 files changed

+30
-19
lines changed

2 files changed

+30
-19
lines changed

activesupport/lib/active_support/notifications/fanout.rb

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,30 @@ def initialize(exceptions)
1717
end
1818

1919
module FanoutIteration # :nodoc:
20-
def iterate_guarding_exceptions(listeners)
21-
exceptions = nil
22-
23-
listeners.each do |s|
24-
yield s
25-
rescue Exception => e
26-
exceptions ||= []
27-
exceptions << e
28-
end
20+
private
21+
def iterate_guarding_exceptions(collection)
22+
exceptions = nil
2923

30-
if exceptions
31-
if exceptions.size == 1
32-
raise exceptions.first
33-
else
34-
raise InstrumentationSubscriberError.new(exceptions), cause: exceptions.first
24+
collection.each do |s|
25+
yield s
26+
rescue Exception => e
27+
exceptions ||= []
28+
exceptions << e
3529
end
36-
end
3730

38-
listeners
39-
end
31+
if exceptions
32+
exceptions = exceptions.flat_map do |exception|
33+
exception.is_a?(InstrumentationSubscriberError) ? exception.exceptions : [exception]
34+
end
35+
if exceptions.size == 1
36+
raise exceptions.first
37+
else
38+
raise InstrumentationSubscriberError.new(exceptions), cause: exceptions.first
39+
end
40+
end
41+
42+
collection
43+
end
4044
end
4145

4246
# This is a default queue implementation that ships with Notifications.
@@ -222,6 +226,8 @@ def groups_for(name) # :nodoc:
222226
# handle.finish
223227
# end
224228
class Handle
229+
include FanoutIteration
230+
225231
def initialize(notifier, name, id, payload) # :nodoc:
226232
@name = name
227233
@id = id
@@ -236,7 +242,7 @@ def start
236242
ensure_state! :initialized
237243
@state = :started
238244

239-
@groups.each do |group|
245+
iterate_guarding_exceptions(@groups) do |group|
240246
group.start(@name, @id, @payload)
241247
end
242248
end
@@ -249,7 +255,7 @@ def finish_with_values(name, id, payload) # :nodoc:
249255
ensure_state! :started
250256
@state = :finished
251257

252-
@groups.each do |group|
258+
iterate_guarding_exceptions(@groups) do |group|
253259
group.finish(name, id, payload)
254260
end
255261
end

activesupport/test/notifications/evented_notification_test.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,18 +126,23 @@ def test_listen_finish_multiple_exception_consistency
126126
listener = Listener.new
127127
notifier.subscribe nil, BadFinishListener.new
128128
notifier.subscribe nil, BadFinishListener.new
129+
notifier.subscribe(nil) { |*args| raise "foo" }
130+
notifier.subscribe(nil) { |obj| raise "foo" }
131+
notifier.subscribe(nil, monotonic: true) { |obj| raise "foo" }
129132
notifier.subscribe nil, listener
130133

131134
notifier.start "hello", 1, {}
132135
notifier.start "world", 1, {}
133136
error = assert_raises InstrumentationSubscriberError do
134137
notifier.finish "world", 1, {}
135138
end
139+
assert_equal 5, error.exceptions.count
136140
assert_instance_of BadListenerException, error.cause
137141

138142
error = assert_raises InstrumentationSubscriberError do
139143
notifier.finish "hello", 1, {}
140144
end
145+
assert_equal 5, error.exceptions.count
141146
assert_instance_of BadListenerException, error.cause
142147

143148
assert_equal 4, listener.events.length

0 commit comments

Comments
 (0)