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

Set enqueue_after_transaction_commit to true and don't allow changing it #301

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

rosa
Copy link
Member

@rosa rosa commented Aug 24, 2024

See the discussion at rails/rails#52659. Adapters should set their own behaviour without making users choose. If users want to choose, they can just use Active Job's configuration. Besides, we're changing the value of this to true to make it harder for people to run into trouble in the future when switching adapters.

@rosa rosa mentioned this pull request Aug 24, 2024
@rosa rosa force-pushed the change-default-for-after-transaction-commit-enqueuing branch from acdfb0f to 0cf64b5 Compare September 2, 2024 12:30
@hms
Copy link
Contributor

hms commented Sep 4, 2024

@rosa

At the risk of sounding a little dense, I'm totally confused by your response above and after reading the linked issue, I feel like I'm only getting to read parts of the conversation and even those bits are out of order.

One of the reasons I've always used database backed ActiveJob backends (delayed_job, good_job, and now solid_queue) is specifically so I get access to ACID behaviors when I require them. I completely understand that once one gets to a given scale (and one requires the speed of a non-database queue or need to split the job system across databases), things get harder, but I really want to push those issues off as far as I can.

A simple Real-world example: activating a new user/customer and sending a welcome email. This is the quintessential example of Job A enqueues Job B and requires almost zero thinking with ACID enqueuing -- the User is Activated and welcomed or not.

Without ACID enqueuing, this requires tracking state and having some form of a watchdog / polling process to detect and manage state transitions.

Given the amount of code between #perform_later and a job being durably enqueued in the after_commit block, there is (relatively speaking) a lot of time for an asynchronous event intervene and is the kind of "heisenbug" that drives developers to drink...

@rosa
Copy link
Member Author

rosa commented Sep 4, 2024

@hms, no worries, not dense! Sorry for the confusion. I kept this PR description short as I preferred to link to the conversation where this was decided. I realise that the conversation is very long and a bit twisted. I think the essence is in this comment. In short, I agree with you about ACID behaviours in these cases and not just that, but simply being able to control exactly when a job is enqueued. However, this is very much against what Rails Core wants to encourage; they want exactly the opposite, and since we're going to propose Solid Queue as default for Rails, this change follows that philosophy.

As for your example of activating + sending a welcome email, you can still achieve this even after this PR is merged. For this, you would just need to set:

class WelcomeCustomerJob < ApplicationJob
  self.enqueue_after_transaction_commit = :never
end

or false instead of :never once those changes to simplify this configuration land in Rails.

We're currently running HEY with

config.active_job.enqueue_after_transaction_commit = :always

and we still opt out of this in specific jobs, not because of ACID advantages (since we run Solid Queue in its own DB after all, so we can't take advantage of this because the committed job would see stale data in the app DB if the main app DB transaction didn't commit) but because, in some cases, we need to know whether the job was enqueued successfully or not.

@rosa
Copy link
Member Author

rosa commented Sep 4, 2024

Also

is the kind of "heisenbug" that drives developers to drink...

😆

@rosa rosa force-pushed the change-default-for-after-transaction-commit-enqueuing branch from 0cf64b5 to 84ab1a5 Compare September 9, 2024 19:03
…ging it

See the discussion in rails/rails#52659.
Adapters should set their own behaviour without making users choose. If
users want to choose, they can just use Active Job's configuration.
Besides, we're changing the value of this to `true` to make it harder
for people to run into trouble in the future when switching adapters.
@rosa rosa force-pushed the change-default-for-after-transaction-commit-enqueuing branch from 84ab1a5 to 60c132e Compare September 16, 2024 11:15
@rosa rosa merged commit fdd7595 into main Sep 16, 2024
8 checks passed
@rosa rosa deleted the change-default-for-after-transaction-commit-enqueuing branch September 16, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants