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

Job adapter setting ignored in seeds #35812

Closed
trcarden opened this Issue Apr 1, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@trcarden
Copy link

commented Apr 1, 2019

Steps to reproduce

  1. Configure a job adapter in application.rb that is NOT :inline
  2. Create seeds file that executes an ActiveJob

Expected behavior

Rails should honor your configuration in application.rb

Actual behavior

Rails forces seeds to execute with the :inline adapter

System configuration

Rails version: 5.2.3
Ruby version: 2.6.2

Why is this a issue

We would like to be able to override the defaults but can't because of this commit: 66cc0e7#diff-1073ed8063cdf047e123bfdefa114422R551

Generally I like the new default however it should not override an adapter if we have one set already. Seeds should not be a "special" context where our configuration is overridden especially when explicitly configured another way.

@trcarden

This comment has been minimized.

Copy link
Author

commented Apr 1, 2019

As a workaround I have placed this at the top of our seeds.rb file which seems to bypass the forced inline configuration and uses the original setting in the application.config

# TODO: Remove me once this is fixed: https://github.com/rails/rails/issues/35812
ActiveSupport.on_load(:active_job) do
  ActiveJob::Base.queue_adapter = Rails.application.config.active_job.queue_adapter
end

If we decide to keep this behavior we should probably update the docs too: https://guides.rubyonrails.org/action_mailer_basics.html#calling-the-mailer (says that the default adapter is :async) as well as some of the comments from here :625baa6

@prathamesh-sonpatki

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@trcarden This is happening because of this change: #34953

The seeding process explicitly uses inline adapter now.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Seeds will execute with inline adapter because they are special. The default is still :async with exception of seeds, so the documentation is still correct.

Only overriding the adapter if it is not set is an option but I believe that will create inconsistence. The job will only execute if you have a different process running in development when your adapter is not async or inline and that means that depending on how you are running the application the state of your database will be different. I'd prefer to have a consistent state and this is what always setting the inline adapter on seed is also trying to accomplish.

@trcarden

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@rafaelfranca If this is intentional ok, it seems inconsistent to me to have the adapter change despite explicitly setting it in application.rb (we don't use any of the built in adapters).

My big question is how do we turn it off if we want to without hacks? We have jobs that run as part of our seeds that we don't want to run inline as they are heavy or reach out to 3rd party systems. That should still be allowed right?

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

If you don't want those jobs to run when seeding you application you should be explicit about it in your code, not relying on them not be executed just because your job runner is not up. If for some reason your job runner is up when running the seeds the jobs will be executed.

You are relying exactly on the inconsistence I said this commit is fixing. So something like:

# db/seed.rb
JobThatIdontWantToRunWhenSeeding.disable!

Would be more intention revealing.

@pwim

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

I'm running into issues due to the combination of this change, and that the inline queue adapter doesn't allow queuing of jobs in the future.

In some of my models I have code like

class SomeModel < ApplicationRecord
  after_create { SomeJob.set(wait_until: 5.minutes.from_now).perform_later(self) }
end

This means I can no longer run my seeds, as it fails with NotImplementedError: Use a queueing backend to enqueue jobs in the future.

These jobs aren't essential to my seeding process, and so could theoretically be skipped, but it doesn't matter if they are performed either, so I'd rather not modify code to aid seeding. I understand my situation is a bit of an edge case, and maybe isn't worth handling cleanly within Rails itself, but I suspect others will probably encounter it too, and it isn't immediately obvious why the queue adapter is different when running the seeds.

My workaround is to set the queue adapter explicitly back to my development queue adapter by placing at the top of my db seeds:

ActiveJob::Base.queue_adapter = Rails.application.config.active_job.queue_adapter

(this is basically what @trcarden is suggesting, but the block for ActiveSupport.on_load is unnecessary)

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

That is a good point. I think we should change to only change the adapter to inline if the adapter is the :async one. Mind to open an PR?

@trcarden

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

@rafaelfranca I think you might be misunderstanding my point. We aren't relying on the job runner not running, we have a completely custom one we want to run in there instead. It's very similar to @pwim above except we have a custom adapter that is not async or inline.

Also if we go all the way back up the stack for why this (#34953) was introduced was because of this: #34939 (it looks like a threading issue). Ironically I have already run into this same problem just a few days ago with @matthewd over here: #34310.

Here is a basic idea of what might be happening
image

However I don't think we should be changing the seeds job runner files to work around a DB shared lock issue. I guess this change set has value in keeping the seeds consistent regardless of the threading issue (especially if we only override for the :async driver).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.