Skip to content
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

Allow to register transaction callbacks outside of a record #51474

Merged
merged 1 commit into from Apr 3, 2024

Conversation

casperisfine
Copy link
Contributor

Ref: #26103
Ref: #51426

A fairly common mistake with Rails is to enqueue a job from inside a transaction, and a record as argumemnt, which then lead to a RecordNotFound error when picked up by the queue.

This is even one of the arguments advanced for job runners backed by the database such as solid_queue, delayed_job or good_job. But relying on this is undesirable iin my opinion as it makes the Active Job abstraction leaky, and if in the future you need to migrate to another backend or even just move the queue to a separate database, you may experience a lot of race conditions of the sort.

But more generally, being able to defer work to after the current transaction has been a missing feature of Active Record. Right now the only way to do it is from a model callback, and this forces moving things in Active Record models that sometimes are better done elsewhere. Even as a self-proclaimed "service object skeptic", I often wanted this capability over the last decade, and I'm sure it got asked or desired by many more people.

Also there's some 3rd party gems adding this capability using monkey patches. It's not a reason to upstream the capability, but it's a proof that there is demand for it.

Implementation wise, this proof of concept shows that it's not really hard to implement, even with nested multi-db transactions support.

One open question is whether we should also expose a callback for after_rollback (and possible after either state). On paper it makes sense, but I kinda fail to see a good use case for it.

cc @matthewd

@casperisfine casperisfine force-pushed the transaction-callback-3 branch 4 times, most recently from 0bf69ce to c9ffc20 Compare April 3, 2024 11:34
@casperisfine
Copy link
Contributor Author

After discussing some more with @matthewd I added:

  • ActiveRecord::Transaction#after_commit, ActiveRecord::Transaction#before_commit, ActiveRecord::Transaction#after_rollback
  • This new class is public and documented with just these 3 methods.
  • ActiveRecord::Base.transaction now yields the current transaction.
  • ``ActiveRecord::Base.current_transactionreturns the current transaction or aNullTransaction`.

With the above there is now a range of ways to get callbacks around transaction lifcycles without having to resort to model callbacks.

I still haven't added an equivalent of ActiveRecord.after_all_transactions_commit for before_commit and after_rollback because IMO the semantic would be confusing. after_all_transactions_commit is the only one that warrant such a generic, high level callback.

activerecord/CHANGELOG.md Show resolved Hide resolved
activerecord/lib/active_record.rb Show resolved Hide resolved
activerecord/test/cases/transactions_test.rb Outdated Show resolved Hide resolved
ite&.each do |i|
i.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: false)
run_action_on_records(ite, instances_to_run_callbacks_on) do |record, should_run_callbacks|
record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
Copy link
Member

Choose a reason for hiding this comment

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

We're not exposing full_rollback? to our new callbacks, but I'm not sure that's a generally useful piece of information... without digging in to history, this feels like it might be facilitating a historical-compatibility quirk rather than a first-principles desired function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I'm not even sure what it means. That the outer transaction is rolled back too?

I don't think we need it.

activerecord/lib/active_record/transaction.rb Show resolved Hide resolved
@casperisfine casperisfine force-pushed the transaction-callback-3 branch 2 times, most recently from 1a8c673 to cd59867 Compare April 3, 2024 12:24
Ref: rails#26103
Ref: rails#51426

A fairly common mistake with Rails is to enqueue a job from inside a
transaction, and a record as argumemnt, which then lead to a RecordNotFound
error when picked up by the queue.

This is even one of the arguments advanced for job runners backed by the
database such as `solid_queue`, `delayed_job` or `good_job`. But relying
on this is undesirable iin my opinion as it makes the Active Job abstraction
leaky, and if in the future you need to migrate to another backend or even
just move the queue to a separate database, you may experience a lot of race
conditions of the sort.

But more generally, being able to defer work to after the current transaction
has been a missing feature of Active Record. Right now the only way to do it
is from a model callback, and this forces moving things in Active Record
models that sometimes are better done elsewhere. Even as a self-proclaimed
"service object skeptic", I often wanted this capability over the last decade,
and I'm sure it got asked or desired by many more people.

Also there's some 3rd party gems adding this capability using monkey patches.
It's not a reason to upstream the capability, but it's a proof that there is
demand for it.

Implementation wise, this proof of concept shows that it's not really hard to
implement, even with nested multi-db transactions support.

Co-Authored-By: Cristian Bica <cristian.bica@gmail.com>
@byroot byroot merged commit 0e0da31 into rails:main Apr 3, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants