-
Notifications
You must be signed in to change notification settings - Fork 9
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
Prepare for Rails 7.2 support (and disable enqueue_after_transaction_commit) #40
Conversation
Other than the fact it would open a second transaction, is there any downside to setting it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domainLGTM
@ghiculescu It's mainly that one downside, which to be fair is a big one 😄. This gem is a foundational building block for ensuring that persistence side effects always run when they should (and don't run when they shouldn't) and, more broadly, for maintaining eventual consistency across systems. It is also consumed by other libraries (like journaled) that rely on these guarantees. So by default we want to maintain all existing message delivery guarantees, and avoid the possibility that some jobs may backslide on Rails 7.2 and lose the DB's ACID compliance. |
Thanks @smudge. That makes sense. I assume if you use a separate database for your jobs, then most of the guarantees you describe go out the window. The readme says this is possible but not explicitly supported, at least that's my interpretation. (We currently use delayed job but are considering switching to |
Since
Correct. There are situations where it may make sense (perhaps your application already connects to two databases), but I would go as far as to say that this is the wrong queuing library if the intention is always to directly enqueue into a secondary database. That said, when pointed at the app's primary database, |
FYI, with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platform lgtm
…or Rails 7.2 support (#43) Hi Betterment friends! As addressed in @smudge's PR #40, Rails 7.2 introduces the concept of `enqueue_after_transaction_commit`, which should not be used in conjunction with Delayed. The implementation, and the associated tests, make the assumption that the values used by ActiveJob are `true` and `false`. The implementation in ActiveJob::EnqueueAfterTransactionCommit (https://github.com/rails/rails/blob/ac1d7681d05906b2b050eced76c6a2165f420821/activejob/lib/active_job/enqueue_after_transaction_commit.rb#L7-L14) uses the values of `:always` and `:never`. The issues arises when hitting the conditional check (https://github.com/Betterment/delayed/compare/main...warmerzega:delayed:main?expand=1#diff-325ce2458ac2c3e05143813a4899c9f996ae51f54cdb8fa5f79f50efab872f9aR20) where `!!:never == true` and causes the condition to be true (and raising the exception) when `job.class.enqueue_after_transaction_commit == :never`.
/domain @effron @samandmoore
/platform @effron @samandmoore
This anticipates the release of a new feature in Rails 7.2 introduced here: rails/rails#51426
In Rails 7.2, ActiveJob will default
enqueue_after_transaction_commit
totrue
, and we would like to explicitly set this tofalse
(retaining transactional enqueues, sincedelayed
is DB-backed).This PR goes a step further and also raises a
UnsafeEnqueueError
if, during enqueue, it encounters any job type that has overriddenenqueue_after_transaction_commit
totrue
. Worth noting that this exception may still be raised after transaction commit (based on the way the lifecycle hooks are handled), so it is intended first and foremost as a guard during development & testing.As part of this change I also: