From 9eb4b4ed01f893c2fc7ff3ee4f99301dec7ea3c2 Mon Sep 17 00:00:00 2001 From: Edouard CHIN Date: Fri, 13 Dec 2019 03:25:03 +0100 Subject: [PATCH] Fix deprecation being thrown at boot time: - ### Problem In rails/rails@bbfab0b33ab I introduced a change which outputs a deprecation whenever a class inherits from ActiveJob::Base. This has the negative effect to trigger a massive amount of deprecation at boot time especially if your app is eagerloaded (like it's usually the case on CI). Another issue with this approach was that the deprecation will be output no matter if a job define a `after_perform` callbacks i.e. ```ruby class MyJob < AJ::Base before_enqueue { throw(:abort) } end # This shouldn't trigger a deprecation since no after callbacks are defined # The change in 6.2 will be already safe for the app. ``` ### Solution Trigger the deprecation only when a job is abort (during enqueuing or performing) AND a `after_perform` callback is defined on the job. --- activejob/lib/active_job/callbacks.rb | 9 ----- activejob/lib/active_job/enqueuing.rb | 8 +++++ activejob/lib/active_job/execution.rb | 14 ++++++++ activejob/test/cases/callbacks_test.rb | 48 +++++++++++++++++++++++--- activejob/test/helper.rb | 1 + 5 files changed, 67 insertions(+), 13 deletions(-) diff --git a/activejob/lib/active_job/callbacks.rb b/activejob/lib/active_job/callbacks.rb index e393da81a2..43943d6fb3 100644 --- a/activejob/lib/active_job/callbacks.rb +++ b/activejob/lib/active_job/callbacks.rb @@ -42,15 +42,6 @@ module ActiveJob # callbacks for +perform+ and +enqueue+ methods. module ClassMethods def inherited(klass) - unless skip_after_callbacks_if_terminated - ActiveSupport::Deprecation.warn(<<~EOM) - In Rails 6.2, ActiveJob's `after_enqueue` and `after_perform` callbacks will no longer run in case the - callback chain is halted (i.e. `throw(:abort)` is thrown in a before_enqueue callback). - To enable this behaviour right now, add in your application configuration file - `config.active_job.skip_after_callbacks_if_terminated = true`. - EOM - end - klass.get_callbacks(:enqueue).config[:skip_after_callbacks_if_terminated] = skip_after_callbacks_if_terminated klass.get_callbacks(:perform).config[:skip_after_callbacks_if_terminated] = skip_after_callbacks_if_terminated super diff --git a/activejob/lib/active_job/enqueuing.rb b/activejob/lib/active_job/enqueuing.rb index cd588de617..02a5420e8d 100644 --- a/activejob/lib/active_job/enqueuing.rb +++ b/activejob/lib/active_job/enqueuing.rb @@ -63,6 +63,14 @@ module ActiveJob if successfully_enqueued self else + if !self.class.skip_after_callbacks_if_terminated && _enqueue_callbacks.any? { |c| c.kind == :after } + ActiveSupport::Deprecation.warn(<<~EOM) + In Rails 6.2, ActiveJob's `after_enqueue` callbacks will no longer run in case the + callback chain is halted (i.e. `throw(:abort)` is thrown in a before_enqueue callback). + To enable this behaviour right now, add in your application configuration file + `config.active_job.skip_after_callbacks_if_terminated = true`. + EOM + end if self.class.return_false_on_aborted_enqueue false else diff --git a/activejob/lib/active_job/execution.rb b/activejob/lib/active_job/execution.rb index e96dbcd4c9..5cf41a4283 100644 --- a/activejob/lib/active_job/execution.rb +++ b/activejob/lib/active_job/execution.rb @@ -35,9 +35,23 @@ module ActiveJob self.executions = (executions || 0) + 1 deserialize_arguments_if_needed + successfully_performed = false + run_callbacks :perform do perform(*arguments) + + successfully_performed = true end + + if !successfully_performed && !self.class.skip_after_callbacks_if_terminated && _perform_callbacks.any? { |c| c.kind == :after } + ActiveSupport::Deprecation.warn(<<~EOM) + In Rails 6.2, ActiveJob's `after_perform` callbacks will no longer run in case the + callback chain is halted (i.e. `throw(:abort)` is thrown in a before_perform callback). + To enable this behaviour right now, add in your application configuration file + `config.active_job.skip_after_callbacks_if_terminated = true`. + EOM + end + successfully_performed rescue => exception rescue_with_handler(exception) || raise end diff --git a/activejob/test/cases/callbacks_test.rb b/activejob/test/cases/callbacks_test.rb index 87dc7634ae..412b7618f9 100644 --- a/activejob/test/cases/callbacks_test.rb +++ b/activejob/test/cases/callbacks_test.rb @@ -27,7 +27,10 @@ class CallbacksTest < ActiveSupport::TestCase test "#enqueue returns false when before_enqueue aborts callback chain and return_false_on_aborted_enqueue = true" do prev = ActiveJob::Base.return_false_on_aborted_enqueue ActiveJob::Base.return_false_on_aborted_enqueue = true - assert_equal false, AbortBeforeEnqueueJob.new.enqueue + + ActiveSupport::Deprecation.silence do + assert_equal false, AbortBeforeEnqueueJob.new.enqueue + end ensure ActiveJob::Base.return_false_on_aborted_enqueue = prev end @@ -48,7 +51,9 @@ class CallbacksTest < ActiveSupport::TestCase ActiveJob::Base.skip_after_callbacks_if_terminated = true reload_job job = AbortBeforeEnqueueJob.new - job.enqueue + ActiveSupport::Deprecation.silence do + job.enqueue + end assert_nil(job.flag) ensure @@ -60,13 +65,31 @@ class CallbacksTest < ActiveSupport::TestCase ActiveJob::Base.skip_after_callbacks_if_terminated = false reload_job job = AbortBeforeEnqueueJob.new - job.enqueue + assert_deprecated(/`after_enqueue` callbacks will no longer run/) do + job.enqueue + end assert_equal("after_enqueue", job.flag) ensure ActiveJob::Base.skip_after_callbacks_if_terminated = prev end + test "#enqueue does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false but job has no after callbacks" do + prev = ActiveJob::Base.skip_after_callbacks_if_terminated + ActiveJob::Base.skip_after_callbacks_if_terminated = false + + job = Class.new(ActiveJob::Base) do + before_enqueue { throw(:abort) } + self.return_false_on_aborted_enqueue = true + end.new + + assert_not_deprecated do + job.enqueue + end + ensure + ActiveJob::Base.skip_after_callbacks_if_terminated = prev + end + test "#perform does not run after_perform callbacks when skip_after_callbacks_if_terminated is true" do prev = ActiveJob::Base.skip_after_callbacks_if_terminated ActiveJob::Base.skip_after_callbacks_if_terminated = true @@ -84,13 +107,30 @@ class CallbacksTest < ActiveSupport::TestCase ActiveJob::Base.skip_after_callbacks_if_terminated = false reload_job job = AbortBeforeEnqueueJob.new - job.perform_now + assert_deprecated(/`after_perform` callbacks will no longer run/) do + job.perform_now + end assert_equal("after_perform", job.flag) ensure ActiveJob::Base.skip_after_callbacks_if_terminated = prev end + test "#perform does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false but job has no after callbacks" do + prev = ActiveJob::Base.skip_after_callbacks_if_terminated + ActiveJob::Base.skip_after_callbacks_if_terminated = false + + job = Class.new(ActiveJob::Base) do + before_perform { throw(:abort) } + end + + assert_not_deprecated do + job.perform_now + end + ensure + ActiveJob::Base.skip_after_callbacks_if_terminated = prev + end + test "#enqueue returns self when the job was enqueued" do job = CallbackJob.new assert_equal job, job.enqueue diff --git a/activejob/test/helper.rb b/activejob/test/helper.rb index 2b1aac8875..d7925aacce 100644 --- a/activejob/test/helper.rb +++ b/activejob/test/helper.rb @@ -12,6 +12,7 @@ if ENV["AJ_INTEGRATION_TESTS"] require "support/integration/helper" else ActiveJob::Base.logger = Logger.new(nil) + ActiveJob::Base.skip_after_callbacks_if_terminated = true require "adapters/#{@adapter}" end