mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fix corrupt transaction state caused by before_commit
exceptions
When a `before_commit` callback raises, the database is rolled back but AR's record of the current transaction is not, leaving the connection in a perpetually broken state that affects all future users of the connection: subsequent requests, jobs, etc. They'll think a transaction is active when none is, so they won't BEGIN on their own. This manifests as missing `after_commit` callbacks and broken ROLLBACKs. This happens because `before_commit` callbacks fire before the current transaction is popped from the stack, but the exception-handling path they hit assumes that the current transaction was already popped. So the database ROLLBACK is issued, but the transaction stack is left intact. Common cause: deadlocked `#touch`, which is now implemented with `before_commit` callbacks. What's next: * We shouldn't allow active transaction state when checking in or out from the connection pool. Verify that conns are clean. * Closer review of txn manager sad paths. Are we missing other spots where we'd end up with incorrect txn state? What's the worst that can happen if txn state drifts? How can we guarantee it doesn't and contain the fallout if it does? Thanks for @tomafro for expert diagnosis!
This commit is contained in:
parent
4f2bce959b
commit
8fd123f7eb
2 changed files with 33 additions and 2 deletions
|
@ -167,8 +167,13 @@ module ActiveRecord
|
||||||
|
|
||||||
def commit_transaction
|
def commit_transaction
|
||||||
transaction = @stack.last
|
transaction = @stack.last
|
||||||
transaction.before_commit_records
|
|
||||||
@stack.pop
|
begin
|
||||||
|
transaction.before_commit_records
|
||||||
|
ensure
|
||||||
|
@stack.pop
|
||||||
|
end
|
||||||
|
|
||||||
transaction.commit
|
transaction.commit
|
||||||
transaction.commit_records
|
transaction.commit_records
|
||||||
end
|
end
|
||||||
|
|
|
@ -34,6 +34,7 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
|
||||||
|
|
||||||
has_many :replies, class_name: "ReplyWithCallbacks", foreign_key: "parent_id"
|
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_commit { |record| record.do_after_commit(nil) }
|
||||||
after_create_commit { |record| record.do_after_commit(:create) }
|
after_create_commit { |record| record.do_after_commit(:create) }
|
||||||
after_update_commit { |record| record.do_after_commit(:update) }
|
after_update_commit { |record| record.do_after_commit(:update) }
|
||||||
|
@ -47,6 +48,12 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
|
||||||
@history ||= []
|
@history ||= []
|
||||||
end
|
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)
|
def after_commit_block(on = nil, &block)
|
||||||
@after_commit ||= {}
|
@after_commit ||= {}
|
||||||
@after_commit[on] ||= []
|
@after_commit[on] ||= []
|
||||||
|
@ -59,6 +66,11 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
|
||||||
@after_rollback[on] << block
|
@after_rollback[on] << block
|
||||||
end
|
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)
|
def do_after_commit(on)
|
||||||
blocks = @after_commit[on] if defined?(@after_commit)
|
blocks = @after_commit[on] if defined?(@after_commit)
|
||||||
blocks.each{|b| b.call(self)} if blocks
|
blocks.each{|b| b.call(self)} if blocks
|
||||||
|
@ -74,6 +86,20 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
|
||||||
@first = TopicWithCallbacks.find(1)
|
@first = TopicWithCallbacks.find(1)
|
||||||
end
|
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
|
def test_call_after_commit_after_transaction_commits
|
||||||
@first.after_commit_block{|r| r.history << :after_commit}
|
@first.after_commit_block{|r| r.history << :after_commit}
|
||||||
@first.after_rollback_block{|r| r.history << :after_rollback}
|
@first.after_rollback_block{|r| r.history << :after_rollback}
|
||||||
|
|
Loading…
Reference in a new issue