Skip to content

Commit

Permalink
GoodJob.restart should not start capsules (job execution) when in a…
Browse files Browse the repository at this point in the history
… webserver but not in async mode (#1055)
  • Loading branch information
bensheldon committed Aug 30, 2023
1 parent 1a70248 commit 16656dc
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 20 deletions.
2 changes: 2 additions & 0 deletions lib/good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ def self.shutdown?
# @param timeout [Numeric] Seconds to wait for active threads to finish.
# @return [void]
def self.restart(timeout: -1)
return if configuration.execution_mode != :async && configuration.in_webserver?

_shutdown_all(Capsule.instances, :restart, timeout: timeout)
end

Expand Down
19 changes: 2 additions & 17 deletions lib/good_job/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,15 @@ def execution_mode
# @return [Boolean]
def execute_async?
execution_mode == :async_all ||
(execution_mode.in?([:async, :async_server]) && in_server_process?)
(execution_mode.in?([:async, :async_server]) && GoodJob.configuration.in_webserver?)
end

# Whether in +:external+ execution mode.
# @return [Boolean]
def execute_externally?
execution_mode.nil? ||
execution_mode == :external ||
(execution_mode.in?([:async, :async_server]) && !in_server_process?)
(execution_mode.in?([:async, :async_server]) && !GoodJob.configuration.in_webserver?)
end

# Whether in +:inline+ execution mode.
Expand All @@ -219,21 +219,6 @@ def async_started?

private

# Whether running in a web server process.
# @return [Boolean, nil]
def in_server_process?
return @_in_server_process if defined? @_in_server_process

@_in_server_process = Rails.const_defined?(:Server) || begin
self_caller = caller

self_caller.grep(%r{config.ru}).any? || # EXAMPLE: config.ru:3:in `block in <main>' OR config.ru:3:in `new_from_string'
self_caller.grep(%r{puma/request}).any? || # EXAMPLE: puma-5.6.4/lib/puma/request.rb:76:in `handle_request'
self_caller.grep(%{/rack/handler/}).any? || # EXAMPLE: iodine-0.7.44/lib/rack/handler/iodine.rb:13:in `start'
(Concurrent.on_jruby? && self_caller.grep(%r{jruby/rack/rails_booter}).any?) # EXAMPLE: uri:classloader:/jruby/rack/rails_booter.rb:83:in `load_environment'
end
end

def send_notify?(active_job)
return false unless GoodJob.configuration.enable_listen_notify
return true unless active_job.respond_to?(:good_job_notify)
Expand Down
20 changes: 18 additions & 2 deletions lib/good_job/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ def self.total_estimated_threads(warn: false)
def initialize(options, env: ENV)
@options = options
@env = env

@_in_webserver = nil
end

def validate!
Expand Down Expand Up @@ -176,7 +178,7 @@ def max_cache

# The number of seconds to wait for jobs to finish when shutting down
# before stopping the thread. +-1+ is forever.
# @return [Numeric]
# @return [Float]
def shutdown_timeout
(
options[:shutdown_timeout] ||
Expand Down Expand Up @@ -248,7 +250,7 @@ def cleanup_preserved_jobs_before_seconds_ago

# Number of jobs a {Scheduler} will execute before automatically cleaning up preserved jobs.
# Positive values will clean up after that many jobs have run, false or 0 will disable, and -1 will clean up after every job.
# @return [Integer, nil]
# @return [Integer, Boolean, nil]
def cleanup_interval_jobs
if rails_config.key?(:cleanup_interval_jobs)
value = rails_config[:cleanup_interval_jobs]
Expand Down Expand Up @@ -352,6 +354,20 @@ def dashboard_default_locale
rails_config[:dashboard_default_locale] || DEFAULT_DASHBOARD_DEFAULT_LOCALE
end

# Whether running in a web server process.
# @return [Boolean, nil]
def in_webserver?
return @_in_webserver unless @_in_webserver.nil?

@_in_webserver = Rails.const_defined?(:Server) || begin
self_caller = caller
self_caller.grep(%r{config.ru}).any? || # EXAMPLE: config.ru:3:in `block in <main>' OR config.ru:3:in `new_from_string'
self_caller.grep(%r{puma/request}).any? || # EXAMPLE: puma-5.6.4/lib/puma/request.rb:76:in `handle_request'
self_caller.grep(%{/rack/handler/}).any? || # EXAMPLE: iodine-0.7.44/lib/rack/handler/iodine.rb:13:in `start'
(Concurrent.on_jruby? && self_caller.grep(%r{jruby/rack/rails_booter}).any?) # EXAMPLE: uri:classloader:/jruby/rack/rails_booter.rb:83:in `load_environment'
end || false
end

private

def rails_config
Expand Down
4 changes: 4 additions & 0 deletions spec/lib/active_job/queue_adapters/good_job_adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
require 'rails_helper'

describe ActiveJob::QueueAdapters::GoodJobAdapter do
before do
GoodJob.configuration.instance_variable_set(:@_in_webserver, nil)
end

it 'inherits from GoodJob::Adapter' do
expect(described_class).to be < GoodJob::Adapter
end
Expand Down
4 changes: 4 additions & 0 deletions spec/lib/good_job/adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
let(:active_job) { instance_double(ActiveJob::Base) }
let(:good_job) { instance_double(GoodJob::Execution, queue_name: 'default', scheduled_at: nil) }

before do
GoodJob.configuration.instance_variable_set(:@_in_webserver, nil)
end

describe '#initialize' do
it 'uses the global configuration value' do
allow(GoodJob.configuration).to receive(:execution_mode).and_return(:external)
Expand Down
12 changes: 12 additions & 0 deletions spec/lib/good_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@
GoodJob::Capsule.new(configuration: configuration)
expect { described_class.restart }.to change(described_class, :shutdown?).from(true).to(false)
end

context 'when in webserver but not in async mode' do
before do
allow(described_class.configuration).to receive(:execution_mode).and_return(:external)
allow(described_class.configuration).to receive(:in_webserver?).and_return(true)
end

it 'does not start capsules' do
GoodJob::Capsule.new(configuration: configuration)
expect { described_class.restart }.not_to change(described_class, :shutdown?).from(true)
end
end
end

describe '.cleanup_preserved_jobs' do
Expand Down
2 changes: 2 additions & 0 deletions spec/support/reset_good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@
end

expect(PgLock.current_database.advisory_lock.others.count).to eq(0), "Existing others advisory locks AFTER test run"

GoodJob.configuration.instance_variable_set(:@_in_webserver, nil)
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/test_app/config/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
# config.action_cable.allowed_request_origins = [ 'http://example.com', /http:\/\/example.*/ ]

# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
# config.force_ssl = true
config.force_ssl = ActiveModel::Type::Boolean.new.cast(ENV["FORCE_SSL"])

# Use the lowest log level to ensure availability of diagnostic information
# when problems arise.
Expand Down

0 comments on commit 16656dc

Please sign in to comment.