Inconsistencies in configuration settings

[paul]

Different config settings seem to behave differently based on how they're defined. I was trying to investigate why GoodJob wasn't preserving completed jobs, even though I have the setting enabled.

In config/initializers/good_job.rb, I have this:

MyApp::Application.configure do
  config.active_job.queue_adapter = :good_job

  config.good_job.tap do |good_job|
    good_job.execution_mode = :external
    good_job.preserve_job_records = true

    good_job.active_record_parent_class = "ApplicationRecord"
    good_job.on_thread_error = ->(exception) { Honeybadger.notify(exception) }

    # Enabled by the procfile on "worker", but on heroku we only want to run it
    # on the first worker dyno
    dyno = ENV["DYNO"]
    if dyno && dyno != "worker.1"
      good_job.enable_cron = false
    end

    good_job.cron = {
      refresh_slack_tokens: {
        cron:  "0 * * * *",
        class: "RefreshSlackAppTokens",
      }
    }
  end
end

I'm enabling preserve_job_records, and its set when I examine the config, but when I look at the value on GoodJob, its not set:

[1] pry(main)> Rails.configuration.good_job
=> {:cron=>{:refresh_slack_tokens=>{:cron=>"0 * * * *", :class=>"RefreshSlackAppTokens"}},
 :execution_mode=>:external,
 :active_record_parent_class=>"ApplicationRecord",
 :on_thread_error=>#<Proc:0x00005607b3af1160 /home/rando/Code/textus/flobot/config/initializers/good_job.rb:12 (lambda)>,
 :preserve_job_records=>true}
[2] pry(main)> GoodJob.preserve_job_records
=> false

Other settings set the same way, like execution_mode, do what I expect and change how GoodJob runs. It seems it allows preserve_job_records to be set on config, but it has no effect. Alternatively, if I attempt to set all these values on GoodJob directly some of them (like GoodJob.execution_mode = :extenal) fail with "undefined method".

It looks like GoodJob defines several mattr_accessors, but which settings are expected to be configured on config, and which on GoodJob directly?

[bensheldon]

Hi @paul. Thank you for opening this issue.

The simple answer is that configuration options should be defined before gem initialization in config/application.rb or config/environments/{environment}.rb. And global options should be put in an initializer config/initializers/good_job.rb that is triggered after gem initialization.

I myself try to organize all configuration inside of an initializer, but this is an area where I haven't figured out a good pattern to allow configuration to be defined after initialization (maybe having GoodJob.reset_configuration!)? I'd love your feedback and experience here.

[paul]

Hi @bensheldon, sorry its taken awhile to get back on this. I supposed I was confused about which options were "global", and which were "config", since config.good_job is still a "global" (there's only one of them in the Rails application). Looking at the docs, its nice that each are broken out into separate sections (which I somehow missed before), but its not obvious why these different options are set in different places.

[bensheldon]

its not obvious why these different options are set in different places.

That's my bad as gem author. Originally there was only the configuration accessors on GoodJob, and then in #199 the Rails config-based configuration was introduced to overcome deadlocks during Rails initialization. They haven't been unified.

My thinking at the moment is that everything should go into the Rails config, but I'd need to build that out and overcome a few issues (like not all config working if it's updated inside of config/initializers/*.rb rather than config/environments/{env}.rb.).

[aried3r]

Hey! I'll use this issue to add my thoughts and our experiences from work as well (coming from #403 (comment)).

In Rails 6.0 we added good_job as a dependency and used an initializer like @paul did using a Rails.application.configure block. This worked as expected and we were able to keep configuration in one file instead of in multiple.

Starting with Rails 6.1 the first issue we encountered was that despite having a block like this in the intializer:

# config/initializers/good_job.rb
Rails.application.configure do
  config.active_job.queue_adapter = :good_job
end

jobs were being executed using the :async adapter. In the console we noticed the following:

Rails.application.config.active_job
=> {
  :queue_adapter => :good_job
}

but

ActiveJob::Base.queue_adapter_name
=> "async"

So the first option we moved to application.rb was config.active_job.queue_adapter. Then jobs that had no delay using .perform_later got executed with GoodJob again, however the rest of the config was not working, most notably no instances of the scheduler and notifier were being started, their .instances were [].
We tried a GoodJob.restart afterwards, in the hopes that this would perhaps reload the config, perhaps re-initialize everything (ok, I admit, I was just hoping this would magically fix it). Looking at this issue here, this is probably the not-yet-existent GoodJob.reset_configuration mentioned here: #380 (comment).

Anyway, I got curious and did some digging, but nothing that matched this behavior directly came up. Between Rails 6.0 and Rails 6.1 there were so many changes, nothing in particular popped out to me. However some issues on the Rails issue tracker seem like others have similar problems.

rails/rails#36650 which links to other issues, but was closed because apparently it's a gem issue, not a Rails issue (which I don't think applies to GoodJob, since it ties in deeply into Rails configuration). However it was later clarified in a code comment in newly generated Rails apps that initializers are just too late to configure the Rails app, see rails/rails#34985.

There's also rails/rails#37270 which I think is not directly related since according to some people it's still unresolved but it got me looking in the right direction.

Also found rails/rails#40552 which feels like it should be related, but I couldn't tell.

TL;DR:

"In general, configuration specific to the application object needs to go either in config/application.rb or in config/environments/#{Rails.env}.rb, you can't just assign to an arbitrary config point in config/initializers and expect it to work." ^{rails/rails#24748 (comment)}

So, we did what GoodJob's README says and put everything into config/application.rb and everything now works as expected. We just left the happy path and perhaps were using undefined behavior in Rails 6.0.

[bensheldon]

@aried3r thank you for digging into it. I think you've linked out to the underlying issue perfectly:

The initializers of railties and engines [GoodJob] generally run before application initializers [config/initializers/good_job.rb]. rails/rails#36650 (comment)

I'm guessing that the change that happened in Rails 6.0/6.1 is that it introduces a loading dependency between ActiveRecord and ActiveJob, which means that ActiveJob is initialized even earlier than previous versions... and once ActiveJob is initialized, so is the adapter, which initializes GoodJob. I put in some logging to verify this:

$ bin/rails s
RUNNING config/application.rb
=> Booting Puma
=> Rails 6.1.4.1 application starting in development
=> Run `bin/rails server --help` for more startup options
RUNNING config/environments/development.rb
INITIALIZING GoodJob::Adapter
RUNNING config/initializers/test.rb
RUNNING GoodJob Railtie config.after_initialize

...which explains it. The GoodJob::Adapter (which also initializes the Scheduler/Notifier/Cronmanager) is initialized before the application initializer, so none of that config is being used.

It's arguable whether this is a workaround or working-as-intended, but Rails.application.config.good_job must be placed in config/application.rb or config/initializers/#{Rails.env}.rb.

I think this is our problem to solve at the moment:

Rails.application.config is not an anti-pattern. Indeed there are a few problems when gems are forcing Rails components to be initialized earlier. Those problems are things that need to be fixed on those gems, not a anti-pattern. rails/rails#36650 (comment)

...so I think there are two things we can do:

1. (faster, but going against many Rails developers habits): Document that Rails.application.config.good_job must be placed in config/application.rb or config/initializers/#{Rails.env}.rb.
2. (longer timeline) Defer initialization of Goodob::Adapter's Schedulers/etc. until config.after_initialize.
3. Deprecate as many of GoodJob.{configuration_method}s as possible in favor of config to reduce confusion.

(and nervously, this makes me wonder whether GoodJob.active_record_parent_class works at all)

[paul]

My 2¢:

Not a fan of 1. I guess its not up to you, but a Rails decision, but if suddenly every gem that hooks into rails has to be configured in application.rb instead of an initializer, then that's going to make for a messy application.rb (or I have put a bunch of things into a new dir like config/application/*.rb and write a custom loader into application.rb that requires them all). Our app currently has 71 files in initializers, basically one for each gem that requires configuration.

I spot checked a few of the gems (omniauth, doorkeeper, activeadmin) plus some other AJ adapters (sidekiq, shoryuken), and they all have their own independent Shoryuken.configure do blocks, likely to sidestep the problem. But, I think most of them predate the Rails facilities for doing this well, which I don't think really matured until 5.2 or so.

All that said, IMO, I think the preferred way for a well-integrated gem is what you outlined in 2: Let Rails handle the config, users define it in an initializer, then finally load Goodjob in an after_initialize hook.

[bensheldon]

Thanks @paul. I agree that 1 is a distasteful workaround/warning. The practical challenge is that Rails may initialize the ActiveJob adapter (which would mean initializing GoodJob) during the boot phase, before the application initializers run.

I think getting to 2 would mean initializing the Adapter in an inactive state, and moving all of these parts in Adapter#initialize to a callable #after_initialize-type method after application initializers have run:

good_job/lib/good_job/adapter.rb

Lines 24 to 41 in 377cde2

	@configuration = GoodJob::Configuration.new(
	{
	execution_mode: execution_mode,
	queues: queues,
	max_threads: max_threads,
	poll_interval: poll_interval,
	}
	)
	@configuration.validate!
	if execute_async? # rubocop:disable Style/GuardClause
	@notifier = GoodJob::Notifier.new
	@poller = GoodJob::Poller.new(poll_interval: @configuration.poll_interval)
	@scheduler = GoodJob::Scheduler.from_configuration(@configuration, warm_cache_on_initialize: Rails.application.initialized?)
	@notifier.recipients << [@scheduler, :create_thread]
	@poller.recipients << [@scheduler, :create_thread]
	@cron_manager = GoodJob::CronManager.new(@configuration.cron_entries, start_on_initialize: Rails.application.initialized?) if @configuration.enable_cron?

Is anyone interested in working on that?

[bensheldon]

I think this problem has been addressed, mainly with #454 and #460. Configuration should now primarily be set via config and it doesn't matter if it's done in an initializer or within {application,environment}.rb files