Remove extra decrement of transaction level #11434

Merged
merged 1 commit into from Jul 15, 2013

Projects

None yet

2 participants

@pftg
Contributor
pftg commented Jul 14, 2013

rollback_active_record_state! tries to restore model state on Exception
by invoking restore_transaction_record_state it decrement deep level by 1.

After restoring state, rollback_active_record_state! ensure that states should be cleared
and level decremented by invoking clear_transaction_record_state,
which cause the bug: because state already reduced in restore_transaction_record_state.

Removed double derement of transaction level
and removed duplicated code which clear transaction state for top level.

Closes: #4566

@pftg
Contributor
pftg commented Jul 14, 2013

I used tests from #9828 but they need some cleanup to use old test models, instead of adding new one.

@spastorino spastorino and 1 other commented on an outdated diff Jul 14, 2013
activerecord/lib/active_record/transactions.rb
@@ -355,13 +356,14 @@ def remember_transaction_record_state #:nodoc:
def clear_transaction_record_state #:nodoc:
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1
@_start_transaction_state.clear if @_start_transaction_state[:level] < 1
+
@spastorino
spastorino Jul 14, 2013 Member

Why was this added? :)

@pftg
pftg Jul 14, 2013 Contributor

Thanks!

@pftg pftg #4566: Remove extra decrement of transaction level
`rollback_active_record_state!` tries to restore model state on `Exception`
by invoking `restore_transaction_record_state` it decrement deep level by `1`.

After restoring it ensure that states to be cleared
and level decremented by invoking `clear_transaction_record_state`,
which cause the bug: because state already reduced in `restore_transaction_record_state`.

Removed double derement of transaction level
and removed duplicated code which clear transaction state for top level.
e0d59e6
@pftg
Contributor
pftg commented Jul 14, 2013

Updated PR with clean tests and fixes based on feedback.

@spastorino spastorino merged commit 8833b82 into rails:master Jul 15, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment