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

Transactional integrity / Job enqueue deferring out of date? #409

Closed
bakku opened this issue Nov 14, 2024 · 7 comments · Fixed by #412
Closed

Transactional integrity / Job enqueue deferring out of date? #409

bakku opened this issue Nov 14, 2024 · 7 comments · Fixed by #412

Comments

@bakku
Copy link
Contributor

bakku commented Nov 14, 2024

Hello!

I wonder whether the transactional integrity documentation / job enqueue deferring is out of date.

The README mentions the following:

Because this can be quite tricky and many people shouldn't need to worry about it, by default Solid Queue is configured in a different database as the main app, job enqueuing is deferred until any ongoing transaction is committed thanks to Active Job's built-in capability to do this.

However, with the default configuration the following code seems to enqueue SomeJob even though the transaction is not successful:

class SomeController < ApplicationController
  # ...

  def create
    SomeModel.transaction do
      SomeModel.create!
      SomeJob.perform_later

      raise ActiveRecord::Rollback
    end

    redirect_to root_path
  end
end

The README then continues to mention the following configuration including a link to the Rails documentation :

config.active_job.enqueue_after_transaction_commit

However, the link does not lead to that configuration setting and it seems like the setting was removed in this PR: rails/rails#53375.

To me it seems the only way to currently defer the enqueuing of a job is to configure it on the job level:

class SomeJob < ApplicationJob
  self.enqueue_after_transaction_commit = true

  def perform
    # ...
  end
end

Am I correct that the docs (and default configuration of solid queue) is out of date?

@rosa
Copy link
Member

rosa commented Nov 14, 2024

Hey @bakku, thanks for raising this!

The README then continues to mention the following configuration including a link to the Rails documentation :

config.active_job.enqueue_after_transaction_commit

However, the link does not lead to that configuration setting and it seems like the setting was removed in this PR: rails/rails#53375.

Ohh, yes, this was true when that option existed, but now it's indeed outdated. I can also see the corresponding method was removed from the queue adapters, which means this is doing nothing in Solid Queue:

def enqueue_after_transaction_commit?
true
end

And also that the default behaviour by Active Job changed as well, as you pointed out: jobs are no longer deferred by default after all transactions commit. So, yes, you're completely correct!

If you want to open a PR to update it, that'd be super great, but if you don't have time I'll take care this week.

@bakku
Copy link
Contributor Author

bakku commented Nov 14, 2024

Hi @rosa, thank you for your response.

I would be happy to create a PR for it, but to be honest I'm not sure I can because I'm not sure how you want solid queue to behave now.

So what I mean is: is it now correct that solid queue doesn't do job enqueue deferring anymore and it should stay this way? Or do you want to change solid queue in a way that it will still offers this by default?

@rosa
Copy link
Member

rosa commented Nov 14, 2024

I would be happy to create a PR for it, but to be honest I'm not sure I can because I'm not sure how you want solid queue to behave now.

It seems I no longer have a choice, no? Active Job no longer lets adapters decide how they want to behave 🤔 Or am I missing something?

@bakku
Copy link
Contributor Author

bakku commented Nov 14, 2024

I guess one potential option would be to change the rails template when solid queue is enabled and to modify the bin/rails solid_queue:install task so the application job looks something like:

class ApplicationJob < ActiveJob::Base
  self.enqueue_after_transaction_commit = true
end

But I never really worked or dabbled deep into the Rails codebase, so not sure whether that's really feasible.

My questions come more from a place of not knowing than knowing 🙂

@rosa
Copy link
Member

rosa commented Nov 14, 2024

Ah, I see what you mean. Yes, I think it should be technically possible to change that default when you install Solid Queue, or even introducing the behaviour within Solid Queue's adapter's enqueue method, but that PR removing the option makes me think Active Job doesn't want adapters to decide this. Otherwise, it'd keep the fallback to the adapter's default. I think it's not within the adapter's purview to do this. Before, it was because adapters were required to implement enqueue_after_transaction_commit?, but now it no longer is.

So I think the README needs to reflect the reality:

  • Active Job sets enqueue_after_transaction_commit to false by default. It can be changed per job (or for all jobs if you do it in ApplicationJob) and we can say how.
  • Solid Queue has no influence there. The only bit that's still true is the separate DB.

@bakku
Copy link
Contributor Author

bakku commented Nov 14, 2024

Alright, I will try to create a PR tomorrow.

@rosa
Copy link
Member

rosa commented Nov 14, 2024

No worries if you don't have time or don't feel like it! No obligations 😊 You were already super helpful just by catching this! 🙏

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 a pull request may close this issue.

2 participants