1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Only wrap subscriber exceptions if there is more than one

Ref: https://github.com/rails/rails/pull/43282
Ref: https://github.com/rails/rails/pull/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.
This commit is contained in:
Jean Boussier 2021-11-02 10:51:53 +01:00
parent cdef2f6a92
commit acd462fc23
4 changed files with 27 additions and 14 deletions

View file

@ -213,11 +213,9 @@ module ActiveRecord
def test_exceptions_from_notifications_are_not_translated def test_exceptions_from_notifications_are_not_translated
original_error = StandardError.new("This StandardError shouldn't get translated") original_error = StandardError.new("This StandardError shouldn't get translated")
subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { raise original_error } subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { raise original_error }
wrapped_error = assert_raises(ActiveSupport::Notifications::InstrumentationSubscriberError) do actual_error = assert_raises(StandardError) do
@connection.execute("SELECT * FROM posts") @connection.execute("SELECT * FROM posts")
end end
actual_error = wrapped_error.exceptions.first
assert_equal original_error, actual_error assert_equal original_error, actual_error
ensure ensure

View file

@ -489,13 +489,11 @@ class QueryCacheTest < ActiveRecord::TestCase
payload[:sql].downcase! payload[:sql].downcase!
end end
error = assert_raises ActiveSupport::Notifications::InstrumentationSubscriberError do assert_raises FrozenError do
ActiveRecord::Base.cache do ActiveRecord::Base.cache do
assert_queries(1) { Task.find(1); Task.find(1) } assert_queries(1) { Task.find(1); Task.find(1) }
end end
end end
assert error.exceptions.first.is_a?(FrozenError)
ensure ensure
ActiveSupport::Notifications.unsubscribe subscriber ActiveSupport::Notifications.unsubscribe subscriber
end end

View file

@ -93,8 +93,16 @@ module ActiveSupport
exceptions ||= [] exceptions ||= []
exceptions << e exceptions << e
end end
ensure
raise InstrumentationSubscriberError.new(exceptions) unless exceptions.nil? if exceptions
if exceptions.size == 1
raise exceptions.first
else
raise InstrumentationSubscriberError.new(exceptions), cause: exceptions.first
end
end
listeners
end end
def listeners_for(name) def listeners_for(name)

View file

@ -92,18 +92,23 @@ module ActiveSupport
], listener.events ], listener.events
end end
def test_listen_start_exception_consistency def test_listen_start_multiple_exception_consistency
notifier = Fanout.new notifier = Fanout.new
listener = Listener.new listener = Listener.new
notifier.subscribe nil, BadStartListener.new notifier.subscribe nil, BadStartListener.new
notifier.subscribe nil, BadStartListener.new
notifier.subscribe nil, listener notifier.subscribe nil, listener
assert_raises InstrumentationSubscriberError do error = assert_raises InstrumentationSubscriberError do
notifier.start "hello", 1, {} notifier.start "hello", 1, {}
end end
assert_raises InstrumentationSubscriberError do assert_instance_of BadListenerException, error.cause
error = assert_raises InstrumentationSubscriberError do
notifier.start "world", 1, {} notifier.start "world", 1, {}
end end
assert_instance_of BadListenerException, error.cause
notifier.finish "world", 1, {} notifier.finish "world", 1, {}
notifier.finish "hello", 1, {} notifier.finish "hello", 1, {}
@ -116,20 +121,24 @@ module ActiveSupport
], listener.events ], listener.events
end end
def test_listen_finish_exception_consistency def test_listen_finish_multiple_exception_consistency
notifier = Fanout.new notifier = Fanout.new
listener = Listener.new listener = Listener.new
notifier.subscribe nil, BadFinishListener.new notifier.subscribe nil, BadFinishListener.new
notifier.subscribe nil, BadFinishListener.new
notifier.subscribe nil, listener notifier.subscribe nil, listener
notifier.start "hello", 1, {} notifier.start "hello", 1, {}
notifier.start "world", 1, {} notifier.start "world", 1, {}
assert_raises InstrumentationSubscriberError do error = assert_raises InstrumentationSubscriberError do
notifier.finish "world", 1, {} notifier.finish "world", 1, {}
end end
assert_raises InstrumentationSubscriberError do assert_instance_of BadListenerException, error.cause
error = assert_raises InstrumentationSubscriberError do
notifier.finish "hello", 1, {} notifier.finish "hello", 1, {}
end end
assert_instance_of BadListenerException, error.cause
assert_equal 4, listener.events.length assert_equal 4, listener.events.length
assert_equal [ assert_equal [