Permalink
Browse files

Restore ActiveRecord states after a rollback for models w/o callbacks

This fixes a regression (#13744) that was caused by 67d8bb9.

In 67d8bb9, we introduced lazy rollback for records, such that the
record's internal states and attributes are not restored immediately
after a transaction rollback, but deferred until they are first
accessed.

This optimization is only performed when the model does not have any
transactional callbacks (e.g. `after_commit` and `after_create`).

Unfortunately, the models used to test the affected codepaths all
comes with some sort of transactional callbacks. Therefore this
codepath remains largely untested until now and as a result there are
a few issues in the implementation that remains hidden until now.

First, the `sync_with_transaction_state` (or more accurately,
`update_attributes_from_transaction_state`) would perform the
synchronization prematurely before a transaction is finalized (i.e.
comitted or rolled back). As a result, when the actuall rollback
happens, the record will incorrectly assumes that its internal states
match the transaction state, and neglect to perform the restore.

Second, `update_attributes_from_transaction_state` calls `committed!`
in some cases. This in turns checks for the `destroyed?` state which
also requires synchronization with the transaction stae, which causes
an infnite recurrsion.

This fix works by deferring the synchronization until the transaction
has been finalized (addressing the first point), and also unrolled
the `committed!` and `rolledback!` logic in-place (addressing the
second point).

It should be noted that the primary purpose of the `committed!` and
`rolledback!` methods are to trigger the relevant transactional
callbacks. Since this code path is only entered when there are no
transactional callbacks on the model, this shouldn't be necessary. By
unrolling the method calls, the intention here (to restore the states
when necessary) becomes more clear.
  • Loading branch information...
chancancode committed Jan 18, 2014
1 parent 35b85d2 commit 7386ffc781fca07a0c656db49fdb54678caef809
@@ -1,3 +1,11 @@
+* ActiveRecord states are now correctly restored after a rollback for
+ models that did not define any transactional callbacks (i.e.
+ `after_commit`, `after_rollback` or `after_create`).
+
+ Fixes #13744.
+
+ *Godfrey Chan*
+
* Make `touch` fire the `after_commit` and `after_rollback` callbacks.
*Harry Brundage*
@@ -23,6 +23,10 @@ def initialize(state = nil)
@parent = nil
end
+ def finalized?
+ @state
+ end
+
def committed?
@state == :committed
end
@@ -397,13 +397,10 @@ def sync_with_transaction_state
end
def update_attributes_from_transaction_state(transaction_state, depth)
- if transaction_state && !has_transactional_callbacks?
+ if transaction_state && transaction_state.finalized? && !has_transactional_callbacks?
unless @reflects_state[depth]
- if transaction_state.committed?
- committed!
- elsif transaction_state.rolledback?
- rolledback!
- end
+ restore_transaction_record_state if transaction_state.rolledback?
+ clear_transaction_record_state
@reflects_state[depth] = true
end
@@ -430,17 +430,26 @@ def test_rollback_when_saving_a_frozen_record
end
def test_restore_active_record_state_for_all_records_in_a_transaction
+ topic_without_callbacks = Class.new(ActiveRecord::Base) do
+ self.table_name = 'topics'
+ end
+
topic_1 = Topic.new(:title => 'test_1')
topic_2 = Topic.new(:title => 'test_2')
+ topic_3 = topic_without_callbacks.new(:title => 'test_3')
+
Topic.transaction do
assert topic_1.save
assert topic_2.save
+ assert topic_3.save
@first.save
@second.destroy
assert topic_1.persisted?, 'persisted'
assert_not_nil topic_1.id
assert topic_2.persisted?, 'persisted'
assert_not_nil topic_2.id
+ assert topic_3.persisted?, 'persisted'
+ assert_not_nil topic_3.id
assert @first.persisted?, 'persisted'
assert_not_nil @first.id
assert @second.destroyed?, 'destroyed'
@@ -451,6 +460,8 @@ def test_restore_active_record_state_for_all_records_in_a_transaction
assert_nil topic_1.id
assert !topic_2.persisted?, 'not persisted'
assert_nil topic_2.id
+ assert !topic_3.persisted?, 'not persisted'
+ assert_nil topic_3.id
assert @first.persisted?, 'persisted'
assert_not_nil @first.id
assert !@second.destroyed?, 'not destroyed'

2 comments on commit 7386ffc

@k00ka

This comment has been minimized.

Show comment Hide comment
@k00ka

k00ka Oct 7, 2015

We are still seeing this issue in versions of Rails containing this code. We're currently running Rails 4.2.3 and have a simple class that is persisted within a transaction, which is then rolled-back. The object maintains an ID and persisted? (==true) state.

k00ka replied Oct 7, 2015

We are still seeing this issue in versions of Rails containing this code. We're currently running Rails 4.2.3 and have a simple class that is persisted within a transaction, which is then rolled-back. The object maintains an ID and persisted? (==true) state.

@sgrif

This comment has been minimized.

Show comment Hide comment
@sgrif

sgrif Oct 7, 2015

Member

@k00ka Please open an issue and provide a script we can use to reproduce the issue using this template, as per our bug report guide found here

Commenting on an almost 2 year old commit is not the proper way to report an issue or get attention on a bug.

Member

sgrif replied Oct 7, 2015

@k00ka Please open an issue and provide a script we can use to reproduce the issue using this template, as per our bug report guide found here

Commenting on an almost 2 year old commit is not the proper way to report an issue or get attention on a bug.

Please sign in to comment.