/rails

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed "fake" nested transactions swallowing manual rollbacks #15017

Closed
wants to merge 3 commits into
base: master
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
4 participants
@chancancode @maxfelsher @arthurnn @rafaelfranca
@chancancode
Member

chancancode commented May 7, 2014

Previously, ActiveRecord::Rollback are silently swallowed by all transaction
blocks. The intention behind this is that these special "exceptions" are only
used to signal a rollback request to Active Record, thus they should be
discarded after the rollback request is handled.

However, this is incorrect for nested transactions. By default, nested
transaction blocks are effectively a no-op – they simply become part of the
parent transaction†. To achieve this, the correct behaviour is to "bubble up"
the ActiveRecord::Rollback "exception", which allow the first parent
transaction block with a real database transaction to correctly handle the
rollback.

The documentation has been changed to reflect this. This effectively rolls
back 53bbbcc.

Fixes #3455, #14698.

† The reason that nested transactions defaults to no-op is that savepoints are
not supported in all databases.

Godfrey Chan

This "bug" was around since forever, so this is definitely a change in behaviour. I doubt that anyone is relying on the current behaviour though.

cc @tenderlove @jeremy @fxn @arthurnn @senny
Show outdated Hide outdated activerecord/test/cases/transactions_test.rb
@first.approved = true
@first.save!
Topic.transaction :requires_new => true do

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 7, 2014

Member

why not use the new hash syntax?(minor)
Copy URL
@arthurnn

arthurnn May 7, 2014

Member

why not use the new hash syntax?(minor)
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 8, 2014

Member

@rafaelfranca do you think it's okay to change this on 4.2?
Member

chancancode commented May 8, 2014

@rafaelfranca do you think it's okay to change this on 4.2?

chancancode added some commits May 7, 2014

@chancancode
Added two failing test cases for nested transactions
13a2079
@chancancode
Fixed "fake" nested transactions swallowing manual rollbacks 
Previously, `ActiveRecord::Rollback` are silently swallowed by all transaction
blocks. The intention behind this is that these special "exceptions" are only
used to signal a rollback request to Active Record, thus they should be
discarded after the rollback request is handled.

However, this is incorrect for nested transactions. By default, nested
transaction blocks are effectively a no-op – they simply become part of the
parent transaction†. To achieve this, the correct behaviour is to "bubble up"
the `ActiveRecord::Rollback` "exception", which allow the first parent
transaction block with a real database transaction to correctly handle the
rollback.

The documentation has been changed to reflect this. This effectively rolls
back 53bbbcc.

Fixes #3455, #14698.

† The reason that nested transactions defaults to no-op is that savepoints are
  not supported in all databases.

*Godfrey Chan*
729a41a
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 8, 2014

Member

😢 I guess this is not that simple then, I'll look at the failures and figure out what's up.
Member

chancancode commented May 8, 2014

😢 I guess this is not that simple then, I'll look at the failures and figure out what's up.

@rafaelfranca rafaelfranca added the activerecord label May 19, 2014

@maxfelsher

This comment has been minimized.

Show comment
Hide comment
@maxfelsher

maxfelsher Oct 8, 2014

Contributor

Does #16284 take care of the issue this was trying to address?
Contributor

maxfelsher commented Oct 8, 2014

Does #16284 take care of the issue this was trying to address?
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Oct 8, 2014

Member

@maxfelsher no, this is a deeper issue that requires more refactoring :/
Member

chancancode commented Oct 8, 2014

@maxfelsher no, this is a deeper issue that requires more refactoring :/

@chancancode chancancode closed this Oct 8, 2014

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Oct 8, 2014

Member

Since this PR is not ready, closing for now. cc @arthurnn
Member

chancancode commented Oct 8, 2014

Since this PR is not ready, closing for now. cc @arthurnn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment