Active Record commit transaction on return, break and throw
#48600
Conversation
casperisfine
force-pushed
the
transaction-commit-on-return
branch
from
4152453
to
e6e2f3d
Compare
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
Show resolved
Hide resolved
casperisfine
force-pushed
the
transaction-commit-on-return
branch
3 times, most recently
from
530f945
to
ebd724b
Compare
|
@matthewd ok, I think I address most if not all the feedback, the only thing is really the |
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
Outdated
Show resolved
Hide resolved
casperisfine
force-pushed
the
transaction-commit-on-return
branch
3 times, most recently
from
bb2b6b7
to
dcc543f
Compare
Fix: rails#45017 Ref: rails#29333 Ref: ruby/timeout#30 Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library started using `throw` to interupt execution which had the adverse effect of committing open transactions. To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer than to potentially commit an incomplete transaction. Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards. However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able to return to its original, less surprising, behavior.
casperisfine
force-pushed
the
transaction-commit-on-return
branch
from
dcc543f
to
5fbaa52
Compare
|
Seems we have a bunch of flaky tests on CI, but I don't think they are related to this PR. |
| If set to `false`, it will be rolled back. | ||
|
|
||
| If set to `true`, the above transaction will be committed. |
There was a problem hiding this comment.
Could we make this more explicit about what will happen? I think this is what happens, but I'm not sure:
If set to `false`, it will be rolled back, so neither model will save.
If set to `true`, the above transaction will be committed. `model` will save, but `other_model` will not.
|
Is it safe to update to |
Yes. |
|
|
||
| | Starting with version | The default value is | | ||
| | --------------------- | -------------------- | | ||
| | (original) | `false` | |
There was a problem hiding this comment.
I'm not sure exactly how to spell it, but perhaps we should distinguish the 6.1 behaviour change in this table somehow, even though the config option under discussion wasn't available at the time? 🤔
Fix: #45017
Ref: #29333
Ref: ruby/timeout#30
Historically only raised errors would trigger a rollback, but in Ruby
2.3, thetimeoutlibrary started usingthrowto interupt execution which had the adverse effect of committing open transactions.To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer than to potentially commit an incomplete transaction.
Using
return,breakorthrowinside atransactionblock was essentially deprecated from Rails 6.1 onwards.However with the release of
timeout 0.4.0,Timeout.timeoutnow raises an error again, and Active Record is able to return to its original, less surprising, behavior.