rails: ActiveRecord silently triggers a rollback when using return in the transaction block in Rails 7
Steps to reproduce
require "bundler/inline"
RAILS_VERSION = "7.0.2.4"
gemfile(true) do
source "https://rubygems.org"
gem "activerecord", RAILS_VERSION
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
require "logger"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :records, force: true do |t|
end
end
class Record < ActiveRecord::Base
end
def transaction_with_return
ActiveRecord::Base.transaction { return Record.create! }
end
def transaction_without_return
ActiveRecord::Base.transaction { Record.create! }
end
class BugTest < Minitest::Test
def setup
Record.delete_all
end
def test_transaction_block_with_return
transaction_with_return
assert_equal 1, Record.count
end
def test_transaction_block_without_return
transaction_without_return
assert_equal 1, Record.count
end
end
Expected behavior
Since using return in the transaction block was deprecated in https://github.com/rails/rails/pull/29333, I would expect that ActiveRecord is not triggering a rollback silently.
Actual behavior
ActiveRecord silently triggers a rollback when return is used in the transaction block.
System configuration
Rails version: 7.0
Ruby version: 3.0
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Reactions: 14
- Comments: 45 (23 by maintainers)
Commits related to this issue
- Rollback transactions when the block returns earlier than expected This behavior change is being announced via a deprecation message since Rails 6.1 and now we are making it the default behavior. — committed to rails/rails by rafaelfranca 3 years ago
- Do not use an exit statement in transactions With ActiveRecord 7.x, transactions are now rolled back whenever they return early. Previously, they were still committed. With `next`, the transactions a... — committed to openSUSE/open-build-service by dmarcoux 2 years ago
- Move return outside of transaction block. A `return` inside causes the transaction to rollback. — committed to culturecode/spatial_features by rywall a year ago
- Active Record commit transaction on `return`, `break` and `throw` Fix: https://github.com/rails/rails/issues/45017 Ref: https://github.com/rails/rails/pull/29333 Ref: https://github.com/ruby/timeout/... — committed to Shopify/rails by byroot a year ago
@dylanahsmith thanks for the context!
I’m worried about my future kids learning Rails and writing perfectly looking Ruby code just to learn the hard way that
returnis sometimes a nono! Jokes aside, I think that no one expected that the deprecation will turn into silent rollbacks.This is a very controversial change, pretty much everyone taking part in the discussion on the deprecation PR raised some concerns about the potential consequences of this change. The only thing that was making it easier to swallow was the promise of making it clear to the user by throwing an exception after the rollback.
Please could we just have a log message that says:
ROLLBACK triggered because of return statement in transaction block?This will give someone something to Google. I just spent a day dismantling a model, trying to find the cause of the silent rollback - taking out every association, every validation, every callback, whittling down all the code in the transaction, only to finally discover that it was
return truethat was the cause of it all.Or yes, an exception!
@matthewd wrote there also that:
And indeed, it was a nasty thing to discover 😬
I just had a moment of great confusion with this new behavior.
Not only is rolling back a transaction on a non erroneous condition very surprising, it does so silently. 😮 This goes against the “Principle of least astonishment”.
And the cause for this change or “workaround” is also questionable. The problem with
Timeout.timoutsounds like something that could have been patched withinTimeout.I don’t like it.
Thoughts on how to get this resolved? Seems sorta dangerous as is.
timeout 0.4.0 was released today: https://rubygems.org/gems/timeout/versions/0.4.0
@zzak yes this is intended behaviour, but we are suggesting improvements, since the behaviour is surprising.
One suggestion is to raise an exception instead of quietly rolling back the transaction: https://github.com/rails/rails/issues/45017#issuecomment-1118134642
Another suggestion is to add a log message so at least someone can come to understand why the rollback happened, without spending a day removing all other code from a block and eventually, somehow, finding their way to this page: https://github.com/rails/rails/issues/45017#issuecomment-1216158244
Right now, all you see in the log is ROLLBACK. There’s no way to know why the rollback happened.
I like the second option more because I think an exception will be difficult to suppress if the early-return rollback is desired.
Yes, now if a 0.4.0 release do happen, we can consider added a dependency on
timeout >= 0.4.0and change the behavior.I want to stress the “consider”, as adding a new dependency in a patch level release isn’t something we normally allow ourselves to do, and this could have knock-on effect on user code (because
Timeout.timeoutwould now be rescuable).I was also caught out by this - the transactions rolled back and since there was no exception risen everything proceeded as usual but the data was lost. I’m against this change altogether - it’s dangerous, not intuitive, and poorly documented.
I was caught out by this today. I would wholeheartedly agree that this is not expected behaviour and I spend a good deal of time reverse engineering the cause.
This change is not only surprising but also dangerous. Essentially this is no better than using an empty
rescueblock to swallow an exception and this goes against every good coding practice.Is there a recommended way to patch Rails to disable this behavior? We are blocked on upgrading to 7 because of this.
Something changed upstream, as https://github.com/ruby/timeout/pull/30 was merged : https://github.com/ruby/timeout/commit/f16545abe68b908f91074086c3166b4dffc57802
How can we bring more attention to this issue? People are losing their data and spend many hours trying to figure out what is happening. Maybe even we could learn something from it to avoid adding more sharp edges to Rails in the future. The series of events that led to this change getting merged is very unfortunate.
@jeremyevans wrote:
That makes sense. Timeout is an error and externally it doesn’t pretend otherwise but implements unwinding internally via a non-error mechanism (throw). That seems to be the flaw, not the original rails transaction behavior. Timeout unwinding should probably be done via a low level exception (below StandardError). There’s already an understanding in ruby that you shouldn’t be swallowing or recovering from those, in a general fashion (
rescue Exception => e).The current transaction behavior is surprising, incoherent and undocumented. For years everyone knew that you aborted transactions with exceptions and only exceptions. So when people intentionally used early exists they obviously weren’t trying to abort.
Does it really make sense that an early transaction block exit via next, commits, but an early block (and method) exit via return, rolls back? What is the fundamental difference that they should do the opposite of each other?
Came here to say that I spent some hours debugging why a transaction was silently rolled back even though everything worked perfectly (including tests) before upgrading to Rails 7.
I didn’t see the deprecation warning because this time I upgraded Rails from 5 -> 7 … I should’ve went one major version at a time 🙄
Both are bad, I wouldn’t be going as far as to say that one is always better than another. Depending on the application, one could be far better with a small inconsistency in their database than with data loss.
Can we at least gain control over whether the exception should be raised or not in our codebases? This is the biggest source of controversy and could help eliminate the element of surprise.
Serious question - if we’re assuming that users can be educated not to use
returnin transactions, why instead of that not educate users on how to use Timeout properly? That way we would avoid any harmful changes to Rails.It’s baffling that there is already a rule in Rubocop warning against using
returnin transactions, but there is no warning on using Timeout.– edit
Adding a warning on Timeout would have an additional benefit - since it’s a non-breaking change, it could be pushed to older Rails versions, together with a Rubocop check helping users fix their codebases. Today we have a situation where prior to Rails 7 something is not working as expected, and after Rails 7 something else is not working as expected.
That code is only called if a second argument is not provided. See https://github.com/ruby/timeout/blob/cae26ed02b8f6dd34a84b04c7c71a844e5a87d48/lib/timeout.rb#L195
@byroot Help me understand - isn’t a solution to the
Timeoutproblem simply passing a second argument?I remember when Rails finally stopped swallowing exceptions in the
after_commitcallback, it’s strange seeing things like this coming back.