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

- ### Problem

  ```ruby
    MyJob < ApplicationJob
      before_enqueue { throw(:abort) }
      after_enqueue { # enters here }
    end
  ```
  I find AJ behaviour on after_enqueue and after_perform callbacks
  weird as they get run even when the callback chain is halted.
  It's counter intuitive to run the after_enqueue callbacks even
  though the job wasn't event enqueued.

  ### Solution

  In Rails 6.2, I propose to make the new behaviour the default
  and stop running after callbacks when the chain is halted.
  For application that wants this behaviour now or in 6.1
  they can do so by adding the `config.active_job.skip_after_callbacks_if_terminated = true`
  in their configuration file.

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"