Merge pull request #37827 from Edouard-chin/ec-activejob-enqueuing

Don't run AJ after_enqueue / after_perform when chain is halted:

activejob/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   Don't run `after_enqueue` and `after_perform` callbacks if the callback chain is halted.
+
+        class MyJob < ApplicationJob
+          before_enqueue { throw(:abort) }
+          after_enqueue { # won't enter here anymore }
+        end
+
+    `after_enqueue` and `after_perform` callbacks will no longer run if the callback chain is halted.
+    This behaviour is a breaking change and won't take effect until Rails 6.2.
+    To enable this behaviour in your app right now, you can add in your app's configuration file
+    `config.active_job.skip_after_callbacks_if_terminated = true`
+
+    *Edouard Chin*
+
 *   Fix enqueuing and performing incorrect logging message.
 
     Jobs will no longer always log "Enqueued MyJob" or "Performed MyJob" when they actually didn't get enqueued/performed.

activejob/lib/active_job/callbacks.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require "active_support/callbacks"
+require "active_support/core_ext/object/with_options"
+require "active_support/core_ext/module/attribute_accessors"
 
 module ActiveJob
   # = Active Job Callbacks
@@ -27,16 +29,33 @@ module ActiveJob
     end
 
     included do
-      define_callbacks :perform
-      define_callbacks :enqueue
+      class_attribute :return_false_on_aborted_enqueue, instance_accessor: false, instance_predicate: false, default: false
+      cattr_accessor :skip_after_callbacks_if_terminated, instance_accessor: false, default: false
 
-      class_attribute :return_false_on_aborted_enqueue, instance_accessor: false, instance_predicate: false
-      self.return_false_on_aborted_enqueue = false
+      with_options(skip_after_callbacks_if_terminated: skip_after_callbacks_if_terminated) do
+        define_callbacks :perform
+        define_callbacks :enqueue
+      end
     end
 
     # These methods will be included into any Active Job object, adding
     # 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
+      end
+
       # Defines a callback that will get called right before the
       # job's perform method is executed.
       #

activejob/test/cases/callbacks_test.rb

@@ -43,8 +43,63 @@ class CallbacksTest < ActiveSupport::TestCase
     ActiveJob::Base.return_false_on_aborted_enqueue = prev
   end
 
+  test "#enqueue does not run after_enqueue 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
+    reload_job
+    job = AbortBeforeEnqueueJob.new
+    job.enqueue
+
+    assert_nil(job.flag)
+  ensure
+    ActiveJob::Base.skip_after_callbacks_if_terminated = prev
+  end
+
+  test "#enqueue does run after_enqueue callbacks when skip_after_callbacks_if_terminated is false" do
+    prev = ActiveJob::Base.skip_after_callbacks_if_terminated
+    ActiveJob::Base.skip_after_callbacks_if_terminated = false
+    reload_job
+    job = AbortBeforeEnqueueJob.new
+    job.enqueue
+
+    assert_equal("after_enqueue", job.flag)
+  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
+    reload_job
+    job = AbortBeforeEnqueueJob.new
+    job.perform_now
+
+    assert_nil(job.flag)
+  ensure
+    ActiveJob::Base.skip_after_callbacks_if_terminated = prev
+  end
+
+  test "#perform does run after_perform callbacks when skip_after_callbacks_if_terminated is false" do
+    prev = ActiveJob::Base.skip_after_callbacks_if_terminated
+    ActiveJob::Base.skip_after_callbacks_if_terminated = false
+    reload_job
+    job = AbortBeforeEnqueueJob.new
+    job.perform_now
+
+    assert_equal("after_perform", job.flag)
+  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
   end
+
+  private
+    def reload_job
+      Object.send(:remove_const, :AbortBeforeEnqueueJob)
+      $LOADED_FEATURES.delete($LOADED_FEATURES.grep(%r{jobs/abort_before_enqueue_job}).first)
+      require "jobs/abort_before_enqueue_job"
+    end
 end

activejob/test/jobs/abort_before_enqueue_job.rb

@@ -4,7 +4,11 @@ class AbortBeforeEnqueueJob < ActiveJob::Base
   MyError = Class.new(StandardError)
 
   before_enqueue :throw_or_raise
+  after_enqueue { self.flag = "after_enqueue" }
   before_perform { throw(:abort) }
+  after_perform { self.flag = "after_perform" }
+
+  attr_accessor :flag
 
   def perform
     raise "This should never be called"

railties/lib/rails/application/configuration.rb

@@ -163,6 +163,10 @@ module Rails
           if respond_to?(:active_storage)
             active_storage.track_variants = true
           end
+
+          if respond_to?(:active_job)
+            active_job.skip_after_callbacks_if_terminated = true
+          end
         else
           raise "Unknown version #{target_version.to_s.inspect}"
         end

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_1.rb.tt

@@ -11,3 +11,5 @@
 
 # Track Active Storage variants in the database.
 # Rails.application.config.active_storage.track_variants = true
+
+# Rails.application.config.active_job.skip_after_callbacks_if_terminated = true

railties/test/application/configuration_test.rb

@@ -2277,6 +2277,21 @@ module ApplicationTests
       assert_equal false, ActiveJob::Base.return_false_on_aborted_enqueue
     end
 
+    test "ActiveJob::Base.skip_after_callbacks_if_terminated is true by default" do
+      app "development"
+
+      assert_equal true, ActiveJob::Base.skip_after_callbacks_if_terminated
+    end
+
+    test "ActiveJob::Base.skip_after_callbacks_if_terminated is false in the 6.0 defaults" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "6.0"'
+
+      app "development"
+
+      assert_equal false, ActiveJob::Base.skip_after_callbacks_if_terminated
+    end
+
     test "ActiveStorage.queues[:analysis] is :active_storage_analysis by default" do
       app "development"