From 1a98e1b590250f965aeb81ebfe4d7c41d2befacf Mon Sep 17 00:00:00 2001 From: "Ben Sheldon [he/him]" Date: Fri, 21 Apr 2023 19:29:06 -0700 Subject: [PATCH] Clean up framework deferred-loading logic to use nested `on_load` blocks; defer loading development gem `activerecord-explain-analyze` (#931) --- .ruby-version | 2 +- Gemfile | 2 +- Gemfile.lock | 2 +- lib/good_job/dependencies.rb | 31 +++---------------- lib/good_job/engine.rb | 19 +++++------- spec/integration/server_spec.rb | 6 ++-- spec/test_app/config/application.rb | 2 ++ spec/test_app/config/environments/test.rb | 2 ++ .../activerecord_explain_analyze.rb | 31 ++++++++++++------- spec/test_app/config/initializers/good_job.rb | 7 ++--- 10 files changed, 44 insertions(+), 60 deletions(-) diff --git a/.ruby-version b/.ruby-version index e4604e3af..be94e6f53 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -3.2.1 +3.2.2 diff --git a/Gemfile b/Gemfile index 87e957d43..f68793157 100644 --- a/Gemfile +++ b/Gemfile @@ -23,7 +23,7 @@ gem 'pg', platforms: [:mri, :mingw, :x64_mingw] gem 'rails' platforms :ruby do - gem "activerecord-explain-analyze" + gem "activerecord-explain-analyze", require: false gem "pry-byebug" gem 'rack-mini-profiler' gem "rbtrace" diff --git a/Gemfile.lock b/Gemfile.lock index 5bc973ef8..c87cbee1e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -466,4 +466,4 @@ DEPENDENCIES yard-activesupport-concern BUNDLED WITH - 2.4.7 + 2.4.12 diff --git a/lib/good_job/dependencies.rb b/lib/good_job/dependencies.rb index b9ef20649..288775418 100644 --- a/lib/good_job/dependencies.rb +++ b/lib/good_job/dependencies.rb @@ -6,40 +6,19 @@ module Dependencies extend ActiveSupport::Concern included do - # @!attribute [rw] _rails_after_initialize_hook_called - # @!scope class - # Whether Railtie.after_initialize has been called yet (default: +false+). - # This will be set on but before +Rails.application.initialize?+ is +true+. - # @return [Boolean] - mattr_accessor :_rails_after_initialize_hook_called, default: false - - # @!attribute [rw] _active_job_loaded - # @!scope class - # Whether ActiveJob has loaded (default: +false+). - # @return [Boolean] - mattr_accessor :_active_job_loaded, default: false - - # @!attribute [rw] _active_record_loaded - # @!scope class - # Whether ActiveRecord has loaded (default: +false+). - # @return [Boolean] - mattr_accessor :_active_record_loaded, default: false + mattr_accessor :_framework_ready, default: false end class_methods do - # Whether GoodJob's has been initialized as of the calling of +Railtie.after_initialize+. - # @return [Boolean] + # Whether Rails framework has sufficiently initialized to enable Async execution. def async_ready? - Rails.application.initialized? || ( - _rails_after_initialize_hook_called && - _active_job_loaded && - _active_record_loaded - ) + Rails.application.initialized? || _framework_ready end - def start_async_adapters + def _start_async_adapters return unless async_ready? + ActiveJob::Base.queue_adapter # Ensure Active Job is initialized GoodJob::Adapter.instances .select(&:execute_async?) .reject(&:async_started?) diff --git a/lib/good_job/engine.rb b/lib/good_job/engine.rb index d1fcd0ec3..f1654f850 100644 --- a/lib/good_job/engine.rb +++ b/lib/good_job/engine.rb @@ -37,26 +37,21 @@ class Engine < ::Rails::Engine initializer "good_job.start_async" do # This hooks into the hookable places during Rails boot, which is unfortunately not Rails.application.initialized? - # If an Adapter is initialized during boot, we want to want to start its async executors once the framework dependencies have loaded. + # If an Adapter is initialized during boot, we want to want to start async executors once the framework dependencies have loaded. # When exactly that happens is out of our control because gems or application code may touch things earlier than expected. # For example, as of Rails 6.1, if an ActiveRecord model is touched during boot, that triggers ActiveRecord to load, # which touches DestroyAssociationAsyncJob, which loads ActiveJob, which may initialize a GoodJob::Adapter, all of which # happens _before_ ActiveRecord finishes loading. GoodJob will deadlock if an async executor is started in the middle of # ActiveRecord loading. - config.after_initialize do ActiveSupport.on_load(:active_record) do - GoodJob._active_record_loaded = true - GoodJob.start_async_adapters - end - - ActiveSupport.on_load(:active_job) do - GoodJob._active_job_loaded = true - GoodJob.start_async_adapters + ActiveSupport.on_load(:active_job) do + GoodJob._framework_ready = true + GoodJob._start_async_adapters + end + GoodJob._start_async_adapters end - - GoodJob._rails_after_initialize_hook_called = true - GoodJob.start_async_adapters + GoodJob._start_async_adapters end end end diff --git a/spec/integration/server_spec.rb b/spec/integration/server_spec.rb index 24d469cc2..cd8966b65 100644 --- a/spec/integration/server_spec.rb +++ b/spec/integration/server_spec.rb @@ -16,7 +16,7 @@ } ShellOut.command("bundle exec rails s -p #{port} -P #{pidfile}", env: env) do |shell| - wait_until(max: 30) do + wait_until(max: 30, increments_of: 0.5) do expect(shell.output).to include(/Listening on/) # In development, GoodJob starts up before Puma redirects logs to stdout @@ -42,7 +42,7 @@ "GOOD_JOB_ENABLE_CRON" => "true", } ShellOut.command("bundle exec rails s -p #{port} -P #{pidfile}", env: env) do |shell| - wait_until(max: 30) do + wait_until(max: 30, increments_of: 0.5) do expect(shell.output).to include(/Listening on/) expect(shell.output).to include(/GoodJob [0-9.]+ started scheduler/) expect(shell.output).to include(/GoodJob started cron/) @@ -56,7 +56,7 @@ "GOOD_JOB_EXECUTION_MODE" => "async", } ShellOut.command('bundle exec rails db:version', env: env) do |shell| - wait_until(max: 30) do + wait_until(max: 30, increments_of: 0.5) do expect(shell.output).to include(/Current version/) end expect(shell.output).not_to include(/GoodJob [0-9.]+ started scheduler/) diff --git a/spec/test_app/config/application.rb b/spec/test_app/config/application.rb index 119556343..3e6f9a555 100644 --- a/spec/test_app/config/application.rb +++ b/spec/test_app/config/application.rb @@ -17,6 +17,8 @@ class Application < Rails::Application # the framework and any gems in your application. # + config.active_job.queue_adapter = :good_job + # config.middleware.insert_before Rack::Sendfile, ActionDispatch::DebugLocks config.log_level = :debug diff --git a/spec/test_app/config/environments/test.rb b/spec/test_app/config/environments/test.rb index bbc811d86..2e04544d9 100644 --- a/spec/test_app/config/environments/test.rb +++ b/spec/test_app/config/environments/test.rb @@ -7,6 +7,8 @@ config.cache_classes = true config.eager_load = true + config.active_job.queue_adapter = :test + # Raises error for missing translations. if Gem::Version.new(Rails.version) < Gem::Version.new('6.1') config.action_view.raise_on_missing_translations = true diff --git a/spec/test_app/config/initializers/activerecord_explain_analyze.rb b/spec/test_app/config/initializers/activerecord_explain_analyze.rb index 068afd31e..873f494f1 100644 --- a/spec/test_app/config/initializers/activerecord_explain_analyze.rb +++ b/spec/test_app/config/initializers/activerecord_explain_analyze.rb @@ -1,18 +1,25 @@ # frozen_string_literal: true -return unless defined? ActiveRecordExplainAnalyze -module ActiveRecordExplainAnalyze - module Relation - alias original_explain explain +ActiveSupport.on_load(:active_record) do + begin + require 'activerecord-explain-analyze' + rescue LoadError + next # ignore + end + + module ActiveRecordExplainAnalyze + module Relation + alias original_explain explain - def explain(analyze: false, format: :text, indexscan: false) - if indexscan - ActiveRecord::Base.connection.execute('SET enable_seqscan = OFF') - result = original_explain(analyze: analyze, format: format) - ActiveRecord::Base.connection.execute('SET enable_seqscan = ON') - result - else - original_explain(analyze: analyze, format: format) + def explain(analyze: false, format: :text, indexscan: false) + if indexscan + ActiveRecord::Base.connection.execute('SET enable_seqscan = OFF') + result = original_explain(analyze: analyze, format: format) + ActiveRecord::Base.connection.execute('SET enable_seqscan = ON') + result + else + original_explain(analyze: analyze, format: format) + end end end end diff --git a/spec/test_app/config/initializers/good_job.rb b/spec/test_app/config/initializers/good_job.rb index fed667883..4da23b8c5 100644 --- a/spec/test_app/config/initializers/good_job.rb +++ b/spec/test_app/config/initializers/good_job.rb @@ -13,7 +13,6 @@ case Rails.env when 'development' - ActiveJob::Base.queue_adapter = :good_job GoodJob.on_thread_error = -> (error) { Rails.logger.warn("#{error}\n#{error.backtrace}") } Rails.application.configure do @@ -44,8 +43,6 @@ when 'test' # test when 'demo' - ActiveJob::Base.queue_adapter = :good_job - Rails.application.configure do config.good_job.execution_mode = :async config.good_job.poll_interval = 30 @@ -81,5 +78,7 @@ } end when 'production' - ActiveJob::Base.queue_adapter = :good_job + # production +else + raise "Unconfigured environment for GoodJob" end