E532 Added flexible eager load configuration for Resque when used in Rails application by georgiybykov · Pull Request #1818 · resque/resque · GitHub
[go: up one dir, main page]

Skip to content

Added flexible eager load configuration for Resque when used in Rails application#1818

Open
georgiybykov wants to merge 1 commit intoresque:masterfrom
georgiybykov:eager_load_config
Open

Added flexible eager load configuration for Resque when used in Rails application#1818
georgiybykov wants to merge 1 commit intoresque:masterfrom
georgiybykov:eager_load_config

Conversation

@georgiybykov
Copy link

Hi!

We faced a problem when upgraded from Resque 1.27.4 to > 2.0.0 version (using Resque gem as a dependency in Rails 6.0.4 application with Ruby 2.7.5) and lost many important events silently. The eager load for Resque task did not work was the reason.

Our production configuration for eager_load is set to true:

# config/environments/production.rb

Rails.application.configure do
  ...
  config.eager_load = true
  ...
end

Changes documentation between versions says the following:

image

image

However, the eager_load will never be true. Since Resque starts from Rake task, the eager_load option configuration is set to false permanently by default starting from this commit at Rails.

Only if your Rails application is 6.1.0 version or above, you are able to configure eager loading for Rake tasks environment by using rake_eager_load option that was added in this commit to provide the ability to override the default false value for every rake task of the environment.

Based on the above, I propose to add flexible configuration for eager_load:

To configure eager_load option using Resque interface you are able to add in config/initializers/resque.rb
the following:

Resque.rails_eager_load_enabled = true # one configuration for any of your application environments

Or if you would like to configure the eager_load option depends on your Rails application environment, for example:

Resque.rails_eager_load_configure do |env|
  env.development = false
  env.staging = true
  env.production = true
end

@georgiybykov georgiybykov force-pushed the eager_load_config branch 2 times, most recently from 6f4d338 to 4f3b12e Compare August 20, 2022 11:34
@iloveitaly
Copy link
Contributor

@georgiybykov Is there a reason you don't do something like:

namespace :resque do
  task :setup => :environment do

  end
end

Pretty sure this would force rails to load before the resque app?

@georgiybykov
Copy link
Author

The file lib/tasks/resque.rake in our Rails application contains the following:

# frozen_string_literal: true

require 'resque/tasks'

namespace :resque do
  task :setup => :environment

  ...
end

The code above is not a solution (does not force preload all classes for the application).

@iloveitaly As an alternative, do you suggest to override the setup task to force preload all classes in memory when booting the app per below?

# frozen_string_literal: true

require 'resque/tasks'

namespace :resque do
  task :setup => :environment

  task :preload => :setup do
    ActiveSupport.run_load_hooks(:before_eager_load, Rails.application)
    Rails.application.config.eager_load_namespaces.each(&:eager_load!)
  end
end

Since Rake tasks ignore the Rails application eager_load configuration setting:

image

eager_load leads to false value here:

image

Only if you use Rails >= 6.1.0, you can impact on this setting via Rails.application.config.rake_eager_load=. However, it will be applied for all rake tasks of you application for current environment.

That is why in this pull request I suggest to add flexible eager load configuration for Resque when used in Rails application without actions to override the code in Rake tasks in order to setup correctly.

@georgiybykov
Copy link
Author

Hi @iloveitaly!

Could you look at this comment as an answer to your question please?

@iloveitaly
Copy link
Contributor

@georgiybykov your explanation makes sense. Tricky problem! I need to set aside some more time to review your PR. Will hopefully get back to you soon.

@georgiybykov
Copy link
Author

Hi @iloveitaly!

Thank you again! I would be very grateful for your feedback and I hope you get a chance to review the code soon! Please let me know if I can provide anything else in the meantime.

@georgiybykov georgiybykov force-pushed the eager_load_config branch 2 times, most recently from c39042f to 5b97aad Compare April 2, 2023 22:25
@fxn
Copy link
fxn commented Apr 15, 2023

I suspect there's a bit of confusion here:

  • In Rake tasks, config.eager_load has been false by default for a long time.
  • In older versions, this was overridden by hand, see Rails 4.0 for example (but users could in turn reassign).
  • In newer version, as it is said above, users can set config.rake_eager_load, whose value will be assigned to config.eager_load before initialization, and it is false by default.

However:

  • If config.eager_load is true, Rails already does what this Rake task is doing.
  • Since idempotence is not a requirement, you'll indeed run stuff twice!

I believe this task should be deleted, though I might be missing something. Let Rails eager load, and let users control this via Rails. I opened #1867 with this proposal.

@georgiybykov
Copy link
Author

@fxn Thank you very much for your comment!

My opinion and the main points of the current pull request are:

  • Prior to the 2.0.0 release change, users did not need to override the task manually to eagerly load the namespaces, as the setup task implemented this behaviour.
  • If we simply delete the code for this task, users will have to override the task in their projects (if they are using Rails with a version below 6.1.0), and they should know about this "feature" by default, but the documentation says nothing about it.
  • In my opinion, even if the application is using one of the latest versions of Rails, not all of them need to enable rake_eager_load which would affect every Rake task just because it uses Resque. So this request is about:
    • removing the confusion from the 2.0.0 release that points to eager_load configuration changes;
    • suggesting to add a more flexible eager load configuration using the Rasque interface.

I believe this task should be deleted, though I might be missing something. Let Rails eager load, and let users control this via Rails. I opened #1867 with this proposal.

Based on the above, you are correct, however the users with Rails version < 6.1.0 should always override the task:

task :preload => :setup do
  ActiveSupport.run_load_hooks(:before_eager_load, Rails.application)
  Rails.application.config.eager_load_namespaces.each(&:eager_load!)
end

because they cannot control this via the Rails configuration.

@dferrazm
Copy link

We faced this issue today using Resque + Wisper. Lots of events were missed on the background jobs.

imho this is a prio A issue since it causes a deviation between the production env in the app server (where eager load is set) and the production evn on the background jobs (where it's not eager loading). People expect that both envs match, but they don't and it's hard to figure that out.

3 years without activity on this PR makes me worry.

I think the answer is to switch to solid_queue these days.

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.

4 participants

0