Skip to content

Commit

Permalink
Rollback transactions when the block returns earlier than expected
Browse files Browse the repository at this point in the history
This behavior change is being announced via a deprecation message
since Rails 6.1 and now we are making it the default behavior.
  • Loading branch information
rafaelfranca committed Nov 19, 2021
1 parent 5a98545 commit 15aa420
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 35 deletions.
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
* Rollback transactions when the block returns earlier than expected.

Before this change, when a transaction block returned early, the transaction would be committed.

The problem is that timeouts triggered inside the transaction block was also making the incomplete transaction
to be committed, so in order to avoid this mistake, the transaction block is rolled back.

*Rafael Mendonça França*

* Add middleware for automatic shard swapping.

Provides a basic middleware to perform automatic shard swapping. Applications will provide a resolver which will determine for an individual request which shard should be used. Example:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,26 +333,19 @@ def within_new_transaction(isolation: nil, joinable: true)
# @connection still holds an open or invalid transaction, so we must not
# put it back in the pool for reuse.
@connection.throw_away! unless transaction.state.rolledback?
elsif Thread.current.status == "aborting" || (!completed && transaction.written)
# The transaction is still open but the block returned earlier.
#
# The block could return early because of a timeout or becase the thread is aborting,
# so we are rolling back to make sure the timeout didn't caused the transaction to be
# committed incompletely.
rollback_transaction
else
if Thread.current.status == "aborting"
rollback_transaction
else
if !completed && transaction.written
ActiveSupport::Deprecation.warn(<<~EOW)
Using `return`, `break` or `throw` to exit a transaction block is
deprecated without replacement. If the `throw` came from
`Timeout.timeout(duration)`, pass an exception class as a second
argument so it doesn't use `throw` to abort its block. This results
in the transaction being committed, but in the next release of Rails
it will rollback.
EOW
end
begin
commit_transaction
rescue Exception
rollback_transaction(transaction) unless transaction.state.completed?
raise
end
begin
commit_transaction
rescue Exception
rollback_transaction(transaction) unless transaction.state.completed?
raise
end
end
end
Expand Down
28 changes: 12 additions & 16 deletions activerecord/test/cases/transactions_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def test_add_to_null_transaction
topic.send(:add_to_transaction)
end

def test_successful_with_return
def test_rollback_with_return
committed = false

Topic.connection.class_eval do
Expand All @@ -195,33 +195,29 @@ def test_successful_with_return
end
end

assert_deprecated do
transaction_with_return
end
assert committed
transaction_with_return
assert_not committed

assert_predicate Topic.find(1), :approved?, "First should have been approved"
assert_not_predicate Topic.find(2), :approved?, "Second should have been unapproved"
assert_not_predicate Topic.find(1), :approved?
assert_predicate Topic.find(2), :approved?
ensure
Topic.connection.class_eval do
remove_method :commit_db_transaction
alias :commit_db_transaction :real_commit_db_transaction rescue nil
end
end

def test_deprecation_on_ruby_timeout
assert_deprecated do
catch do |timeout|
Topic.transaction do
@first.approved = true
@first.save!
def test_rollback_on_ruby_timeout
catch do |timeout|
Topic.transaction do
@first.approved = true
@first.save!

throw timeout
end
throw timeout
end
end

assert Topic.find(1).approved?, "First should have been approved"
assert_not_predicate Topic.find(1), :approved?
end

def test_early_return_from_transaction
Expand Down
7 changes: 7 additions & 0 deletions guides/source/7_0_release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ Please refer to the [Changelog][active-record] for detailed changes.

### Notable changes

* Rollback transactions when the block returns earlier than expected.

Before this change, when a transaction block returned early, the transaction would be committed.

The problem is that timeouts triggered inside the transaction block was also making the incomplete transaction
to be committed, so in order to avoid this mistake, the transaction block is rolled back.

Active Storage
--------------

Expand Down

3 comments on commit 15aa420

@joevandyk
Copy link
Contributor

@joevandyk joevandyk commented on 15aa420 Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #45017

ActiveRecord silently triggers a rollback when return is used in the transaction block.

@joevandyk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to emit a log or something when this happens, instead of silently rolling back the transaction?

@zzak
Copy link
Member

@zzak zzak commented on 15aa420 Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joevandyk Would you be interested in submitting a PR?

I don't want to encourage you to invest time in something that might not be accepted, but in my experience it's easier to get feedback by submitting a patch than it is to comment on old commits. Thank you for understanding!

Please sign in to comment.