From bbfab0b33abd7481b8a801b672d7b2316609315a Mon Sep 17 00:00:00 2001 From: Edouard CHIN Date: Thu, 28 Nov 2019 13:56:49 +0100 Subject: [PATCH] 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 | 14 +++++ activejob/lib/active_job/callbacks.rb | 27 +++++++-- activejob/test/cases/callbacks_test.rb | 55 +++++++++++++++++++ .../test/jobs/abort_before_enqueue_job.rb | 4 ++ .../lib/rails/application/configuration.rb | 4 ++ .../new_framework_defaults_6_1.rb.tt | 2 + .../test/application/configuration_test.rb | 15 +++++ 7 files changed, 117 insertions(+), 4 deletions(-) diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index 1d3e89c184..644d5d79a3 100644 --- a/activejob/CHANGELOG.md +++ b/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. diff --git a/activejob/lib/active_job/callbacks.rb b/activejob/lib/active_job/callbacks.rb index 6b82ea6cab..e393da81a2 100644 --- a/activejob/lib/active_job/callbacks.rb +++ b/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 @@ class << self 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. # diff --git a/activejob/test/cases/callbacks_test.rb b/activejob/test/cases/callbacks_test.rb index 895edb34a5..87dc7634ae 100644 --- a/activejob/test/cases/callbacks_test.rb +++ b/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 diff --git a/activejob/test/jobs/abort_before_enqueue_job.rb b/activejob/test/jobs/abort_before_enqueue_job.rb index 8fb4d8078c..24206abd2e 100644 --- a/activejob/test/jobs/abort_before_enqueue_job.rb +++ b/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" diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 5189698d73..d57a86006d 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -163,6 +163,10 @@ def load_defaults(target_version) 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 diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_1.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_1.rb.tt index cf534ac1f8..e161bf6593 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_1.rb.tt +++ b/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 diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 4de07ebb47..7084185a33 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2277,6 +2277,21 @@ class ::DummySerializer < ActiveJob::Serializers::ObjectSerializer; end 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" -- GitLab