Merge pull request #23407 from jeremy/corrupt-before-commit

Fix corrupt transaction state caused by `before_commit` exceptions

2 files changed, 33 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index 14d04a6388..6ecdab6eb0 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -167,8 +167,13 @@ module ActiveRecord
 
       def commit_transaction
         transaction = @stack.last
-        transaction.before_commit_records
-        @stack.pop
+
+        begin
+          transaction.before_commit_records
+        ensure
+          @stack.pop
+        end
+
         transaction.commit
         transaction.commit_records
       end
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 637f89196e..8a7f19293d 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -34,6 +34,7 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
 
     has_many :replies, class_name: "ReplyWithCallbacks", foreign_key: "parent_id"
 
+    before_commit { |record| record.do_before_commit(nil) }
     after_commit { |record| record.do_after_commit(nil) }
     after_create_commit { |record| record.do_after_commit(:create) }
     after_update_commit { |record| record.do_after_commit(:update) }
@@ -47,6 +48,12 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
       @history ||= []
     end
 
+    def before_commit_block(on = nil, &block)
+      @before_commit ||= {}
+      @before_commit[on] ||= []
+      @before_commit[on] << block
+    end
+
     def after_commit_block(on = nil, &block)
       @after_commit ||= {}
       @after_commit[on] ||= []
@@ -59,6 +66,11 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
       @after_rollback[on] << block
     end
 
+    def do_before_commit(on)
+      blocks = @before_commit[on] if defined?(@before_commit)
+      blocks.each{|b| b.call(self)} if blocks
+    end
+
     def do_after_commit(on)
       blocks = @after_commit[on] if defined?(@after_commit)
       blocks.each{|b| b.call(self)} if blocks
@@ -74,6 +86,20 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     @first = TopicWithCallbacks.find(1)
   end
 
+  # FIXME: Test behavior, not implementation.
+  def test_before_commit_exception_should_pop_transaction_stack
+    @first.before_commit_block { raise 'better pop this txn from the stack!' }
+
+    original_txn = @first.class.connection.current_transaction
+
+    begin
+      @first.save!
+      fail
+    rescue
+      assert_equal original_txn, @first.class.connection.current_transaction
+    end
+  end
+
   def test_call_after_commit_after_transaction_commits
     @first.after_commit_block{|r| r.history << :after_commit}
     @first.after_rollback_block{|r| r.history << :after_rollback}