Merge pull request #16537 from arthurnn/stop_swallowing_errors_2

Add option to stop swallowing errors on callbacks.

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   Currently, Active Record will rescue any errors raised within
+    after_rollback/after_create callbacks and print them to the logs. Next versions of rails
+    will not rescue those errors anymore, and just bubble them up, as the other callbacks.
+
+    This adds a opt-in flag to enable that behaviour, of not rescuing the errors.
+    Example:
+
+        # For not swallow errors in after_commit/after_rollback callbacks.
+        config.active_record.errors_in_transactional_callbacks = true
+
+    Fixes #13460.
+
+    *arthurnn*
+
 *   Fixed an issue where custom accessor methods (such as those generated by
     `enum`) with the same name as a global method are incorrectly overridden
     when subclassing.

activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

@@ -22,6 +22,10 @@ def rolledback?
         @state == :rolledback
       end
 
+      def completed?
+        committed? || rolledback?
+      end
+
       def set_state(state)
         if !VALID_STATES.include?(state)
           raise ArgumentError, "Invalid transaction state: #{state}"
@@ -63,27 +67,39 @@ def rollback
       end
 
       def rollback_records
-        records.uniq.each do |record|
+        ite = records.uniq
+        while record = ite.shift
           begin
             record.rolledback! full_rollback?
           rescue => e
+            raise if ActiveRecord::Base.raise_in_transactional_callbacks
             record.logger.error(e) if record.respond_to?(:logger) && record.logger
           end
         end
+      ensure
+        ite.each do |i|
+          i.rolledback!(full_rollback?, false)
+        end
       end
 
       def commit
         @state.set_state(:committed)
       end
 
       def commit_records
-        records.uniq.each do |record|
+        ite = records.uniq
+        while record = ite.shift
           begin
             record.committed!
           rescue => e
+            raise if ActiveRecord::Base.raise_in_transactional_callbacks
             record.logger.error(e) if record.respond_to?(:logger) && record.logger
           end
         end
+      ensure
+        ite.each do |i|
+          i.committed!(false)
+        end
       end
 
       def full_rollback?; true; end
@@ -103,14 +119,14 @@ def initialize(connection, savepoint_name, options)
       end
 
       def rollback
-        super
         connection.rollback_to_savepoint(savepoint_name)
+        super
         rollback_records
       end
 
       def commit
-        super
         connection.release_savepoint(savepoint_name)
+        super
         parent = connection.transaction_manager.current_transaction
         records.each { |r| parent.add_record(r) }
       end
@@ -130,14 +146,14 @@ def initialize(connection, options)
       end
 
       def rollback
-        super
         connection.rollback_db_transaction
+        super
         rollback_records
       end
 
       def commit
-        super
         connection.commit_db_transaction
+        super
         commit_records
       end
     end
@@ -177,7 +193,7 @@ def within_new_transaction(options = {})
         begin
           commit_transaction unless error
         rescue Exception
-          transaction.rollback
+          transaction.rollback unless transaction.state.completed?
           raise
         end
       end

activerecord/lib/active_record/transactions.rb

@@ -3,11 +3,23 @@ module ActiveRecord
   module Transactions
     extend ActiveSupport::Concern
     ACTIONS = [:create, :destroy, :update]
+    CALLBACK_WARN_MESSAGE = <<-EOF
+Currently, Active Record will rescue any errors raised within
+after_rollback/after_create callbacks and print them to the logs. In the next
+version, these errors will no longer be rescued. Instead, they will simply
+bubble just like other Active Record callbacks.
+
+You can opt into the new behavior and remove this warning by setting
+config.active_record.raise_in_transactional_callbacks to true.
+EOF
 
     included do
       define_callbacks :commit, :rollback,
                        terminator: ->(_, result) { result == false },
                        scope: [:kind, :name]
+
+      mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
+      self.raise_in_transactional_callbacks = false
     end
 
     # = Active Record Transactions
@@ -223,6 +235,9 @@ def transaction(options = {}, &block)
       def after_commit(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:commit, :after, *args, &block)
+        unless ActiveRecord::Base.raise_in_transactional_callbacks
+          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
+        end
       end
 
       # This callback is called after a create, update, or destroy are rolled back.
@@ -231,6 +246,9 @@ def after_commit(*args, &block)
       def after_rollback(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:rollback, :after, *args, &block)
+        unless ActiveRecord::Base.raise_in_transactional_callbacks
+          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
+        end
       end
 
       private
@@ -290,16 +308,16 @@ def rollback_active_record_state!
     #
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
-    def committed! #:nodoc:
-      run_callbacks :commit if destroyed? || persisted?
+    def committed!(should_run_callbacks = true) #:nodoc:
+      run_callbacks :commit if should_run_callbacks && destroyed? || persisted?
     ensure
       force_clear_transaction_record_state
     end
 
     # Call the +after_rollback+ callbacks. The +force_restore_state+ argument indicates if the record
     # state should be rolled back to the beginning or just to the last savepoint.
-    def rolledback!(force_restore_state = false) #:nodoc:
-      run_callbacks :rollback
+    def rolledback!(force_restore_state = false, should_run_callbacks = true) #:nodoc:
+      run_callbacks :rollback if should_run_callbacks
     ensure
       restore_transaction_record_state(force_restore_state)
       clear_transaction_record_state

activerecord/test/cases/helper.rb

@@ -24,6 +24,9 @@
 # Disable available locale checks to avoid warnings running the test suite.
 I18n.enforce_available_locales = false
 
+# Enable raise errors in after_commit and after_rollback.
+ActiveRecord::Base.raise_in_transactional_callbacks = true
+
 # Connect to the database
 ARTest.connect
 

activerecord/test/cases/transaction_callbacks_test.rb

@@ -267,6 +267,9 @@ def @first.commits(i=0); @commits ||= 0; @commits += i if i; end
   end
 
   def test_after_transaction_callbacks_should_prevent_callbacks_from_being_called
+    old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks
+    ActiveRecord::Base.raise_in_transactional_callbacks = false
+
     def @first.last_after_transaction_error=(e); @last_transaction_error = e; end
     def @first.last_after_transaction_error; @last_transaction_error; end
     @first.after_commit_block{|r| r.last_after_transaction_error = :commit; raise "fail!";}
@@ -291,6 +294,79 @@ def @first.last_after_transaction_error; @last_transaction_error; end
     end
     assert_equal :rollback, @first.last_after_transaction_error
     assert_equal [:after_rollback], second.history
+  ensure
+    ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config
+  end
+
+  def test_after_commit_should_not_raise_when_raise_in_transactional_callbacks_false
+    old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks
+    ActiveRecord::Base.raise_in_transactional_callbacks = false
+    @first.after_commit_block{ fail "boom" }
+    Topic.transaction { @first.save! }
+  ensure
+    ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config
+  end
+
+  def test_after_commit_callback_should_not_swallow_errors
+    @first.after_commit_block{ fail "boom" }
+    assert_raises(RuntimeError) do
+      Topic.transaction do
+        @first.save!
+      end
+    end
+  end
+
+  def test_after_commit_callback_when_raise_should_not_restore_state
+    first = TopicWithCallbacks.new
+    second = TopicWithCallbacks.new
+    first.after_commit_block{ fail "boom" }
+    second.after_commit_block{ fail "boom" }
+
+    begin
+      Topic.transaction do
+        first.save!
+        assert_not_nil first.id
+        second.save!
+        assert_not_nil second.id
+      end
+    rescue
+    end
+    assert_not_nil first.id
+    assert_not_nil second.id
+    assert first.reload
+  end
+
+  def test_after_rollback_callback_should_not_swallow_errors_when_set_to_raise
+    error_class = Class.new(StandardError)
+    @first.after_rollback_block{ raise error_class }
+    assert_raises(error_class) do
+      Topic.transaction do
+        @first.save!
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+
+  def test_after_rollback_callback_when_raise_should_restore_state
+    error_class = Class.new(StandardError)
+
+    first = TopicWithCallbacks.new
+    second = TopicWithCallbacks.new
+    first.after_rollback_block{ raise error_class }
+    second.after_rollback_block{ raise error_class }
+
+    begin
+      Topic.transaction do
+        first.save!
+        assert_not_nil first.id
+        second.save!
+        assert_not_nil second.id
+        raise ActiveRecord::Rollback
+      end
+    rescue error_class
+    end
+    assert_nil first.id
+    assert_nil second.id
   end
 
   def test_after_rollback_callbacks_should_validate_on_condition