Fix ActiveJob::EnqueueAfterTransactionCommit
API
Fix: https://github.com/rails/rails/pull/51426#issuecomment-2042611790 `perform_later` is supposed to return the Job instance on success, and `false` on error. When the `enqueue` is automatically delayed, it's of course impossible to predict if the actual queueing will succeed, but for backward compatibility reasons, it's best to assume it will. If necessary, you can hold onto the job instance and check for `#successfully_enqueued?` after the transaction has completed.
This commit is contained in:
parent
61d78da043
commit
afa019835b
@ -16,9 +16,11 @@ module EnqueueAfterTransactionCommit # :nodoc:
|
||||
# - `:never` forces the job to be queued immediately.
|
||||
# - `:default` lets the queue adapter define the behavior (recommended).
|
||||
class_attribute :enqueue_after_transaction_commit, instance_accessor: false, instance_predicate: false, default: :never
|
||||
end
|
||||
|
||||
around_enqueue do |job, block|
|
||||
after_transaction = case job.class.enqueue_after_transaction_commit
|
||||
private
|
||||
def raw_enqueue
|
||||
after_transaction = case self.class.enqueue_after_transaction_commit
|
||||
when :always
|
||||
true
|
||||
when :never
|
||||
@ -28,11 +30,15 @@ module EnqueueAfterTransactionCommit # :nodoc:
|
||||
end
|
||||
|
||||
if after_transaction
|
||||
ActiveRecord.after_all_transactions_commit(&block)
|
||||
self.successfully_enqueued = true
|
||||
ActiveRecord.after_all_transactions_commit do
|
||||
self.successfully_enqueued = false
|
||||
super
|
||||
end
|
||||
self
|
||||
else
|
||||
block.call
|
||||
super
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -50,14 +50,17 @@ module ClassMethods
|
||||
# custom serializers.
|
||||
#
|
||||
# Returns an instance of the job class queued with arguments available in
|
||||
# Job#arguments or false if the enqueue did not succeed.
|
||||
# Job#arguments or +false+ if the enqueue did not succeed.
|
||||
#
|
||||
# After the attempted enqueue, the job will be yielded to an optional block.
|
||||
#
|
||||
# If Active Job is used conjointly with Active Record, and #perform_later is called
|
||||
# inside an Active Record transaction, then the enqueue is implicitly deferred to after
|
||||
# the transaction is committed, or droped if it's rolled back. This behavior can
|
||||
# be changed on a per job basis:
|
||||
# the transaction is committed, or droped if it's rolled back. In such case #perform_later
|
||||
# will return the job instance like if it was successfully enqueued, but will still return
|
||||
# +false+ if a callback prevented the job from being enqueued.
|
||||
#
|
||||
# This behavior can be changed on a per job basis:
|
||||
#
|
||||
# class NotificationJob < ApplicationJob
|
||||
# self.enqueue_after_transaction_commit = false
|
||||
@ -98,6 +101,18 @@ def enqueue(options = {})
|
||||
self.successfully_enqueued = false
|
||||
|
||||
run_callbacks :enqueue do
|
||||
raw_enqueue
|
||||
end
|
||||
|
||||
if successfully_enqueued?
|
||||
self
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
def raw_enqueue
|
||||
if scheduled_at
|
||||
queue_adapter.enqueue_at self, scheduled_at.to_f
|
||||
else
|
||||
@ -108,12 +123,5 @@ def enqueue(options = {})
|
||||
rescue EnqueueError => e
|
||||
self.enqueue_error = e
|
||||
end
|
||||
|
||||
if successfully_enqueued?
|
||||
self
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
105
activejob/test/cases/enqueue_after_transaction_commit_test.rb
Normal file
105
activejob/test/cases/enqueue_after_transaction_commit_test.rb
Normal file
@ -0,0 +1,105 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
require "helper"
|
||||
require "jobs/enqueue_error_job"
|
||||
|
||||
class EnqueueAfterTransactionCommitTest < ActiveSupport::TestCase
|
||||
class FakeActiveRecord
|
||||
attr_reader :calls
|
||||
|
||||
def initialize(should_yield = true)
|
||||
@calls = 0
|
||||
@yield = should_yield
|
||||
@callbacks = []
|
||||
end
|
||||
|
||||
def after_all_transactions_commit(&block)
|
||||
@calls += 1
|
||||
if @yield
|
||||
yield
|
||||
else
|
||||
@callbacks << block
|
||||
end
|
||||
end
|
||||
|
||||
def run_after_commit_callbacks
|
||||
callbacks, @callbacks = @callbacks, []
|
||||
callbacks.each(&:call)
|
||||
end
|
||||
end
|
||||
|
||||
class EnqueueAfterCommitJob < ActiveJob::Base
|
||||
self.enqueue_after_transaction_commit = :always
|
||||
|
||||
def perform
|
||||
# noop
|
||||
end
|
||||
end
|
||||
|
||||
class ErrorEnqueueAfterCommitJob < EnqueueErrorJob
|
||||
class EnqueueErrorAdapter
|
||||
def enqueue_after_transaction_commit?
|
||||
true
|
||||
end
|
||||
|
||||
def enqueue(...)
|
||||
raise ActiveJob::EnqueueError, "There was an error enqueuing the job"
|
||||
end
|
||||
|
||||
def enqueue_at(...)
|
||||
raise ActiveJob::EnqueueError, "There was an error enqueuing the job"
|
||||
end
|
||||
end
|
||||
|
||||
self.queue_adapter = EnqueueErrorAdapter.new
|
||||
self.enqueue_after_transaction_commit = :always
|
||||
|
||||
def perform
|
||||
# noop
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later wait for transactions to complete before enqueuing the job" do
|
||||
fake_active_record = FakeActiveRecord.new
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
assert_difference -> { fake_active_record.calls }, +1 do
|
||||
EnqueueAfterCommitJob.perform_later
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later returns the Job instance even if it's delayed by `after_all_transactions_commit`" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
job = EnqueueAfterCommitJob.perform_later
|
||||
assert_instance_of EnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later yields the enqueued Job instance even if it's delayed by `after_all_transactions_commit`" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
called = false
|
||||
job = EnqueueAfterCommitJob.perform_later do |yielded_job|
|
||||
called = true
|
||||
assert_instance_of EnqueueAfterCommitJob, yielded_job
|
||||
end
|
||||
assert called, "#perform_later yielded the job"
|
||||
assert_instance_of EnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later assumes successful enqueue, but update status later" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
job = ErrorEnqueueAfterCommitJob.perform_later
|
||||
assert_instance_of ErrorEnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
|
||||
fake_active_record.run_after_commit_callbacks
|
||||
assert_not_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
end
|
@ -23,3 +23,5 @@ def adapter_is?(*adapter_class_symbols)
|
||||
end
|
||||
|
||||
require_relative "../../tools/test_common"
|
||||
|
||||
ActiveJob::Base.include(ActiveJob::EnqueueAfterTransactionCommit)
|
||||
|
@ -1,3 +1,7 @@
|
||||
* `stub_const` now accepts a `exists: false` parameter to allow stubbing missing constants.
|
||||
|
||||
*Jean Boussier*
|
||||
|
||||
* Make ActiveSupport::BacktraceCleaner copy filters and silencers on dup and clone
|
||||
|
||||
Previously the copy would still share the internal silencers and filters array,
|
||||
|
@ -15,17 +15,39 @@ module ConstantStubbing
|
||||
# Using this method rather than forcing <tt>World::List::Import::LARGE_IMPORT_THRESHOLD = 5000</tt> prevents
|
||||
# warnings from being thrown, and ensures that the old value is returned after the test has completed.
|
||||
#
|
||||
# If the constant doesn't already exists, but you need it set for the duration of the block
|
||||
# you can do so by passing `exists: false`.
|
||||
#
|
||||
# stub_const(object, :SOME_CONST, 1, exists: false) do
|
||||
# assert_equal 1, SOME_CONST
|
||||
# end
|
||||
#
|
||||
# Note: Stubbing a const will stub it across all threads. So if you have concurrent threads
|
||||
# (like separate test suites running in parallel) that all depend on the same constant, it's possible
|
||||
# divergent stubbing will trample on each other.
|
||||
def stub_const(mod, constant, new_value)
|
||||
old_value = mod.const_get(constant, false)
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, old_value)
|
||||
def stub_const(mod, constant, new_value, exists: true)
|
||||
if exists
|
||||
begin
|
||||
old_value = mod.const_get(constant, false)
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, old_value)
|
||||
end
|
||||
else
|
||||
if mod.const_defined?(constant)
|
||||
raise NameError, "already defined constant #{constant} in #{mod.name}"
|
||||
end
|
||||
|
||||
begin
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -580,7 +580,7 @@ class TestConstStubbing < ActiveSupport::TestCase
|
||||
assert_equal 1, ConstStubbable::CONSTANT
|
||||
end
|
||||
|
||||
test "trying to stub a constant that does not exist in the receiver raises NameError" do
|
||||
test "stubbing a constant that does not exist in the receiver raises NameError" do
|
||||
assert_raises(NameError) do
|
||||
stub_const(ConstStubbable, :NOT_A_CONSTANT, 1) { }
|
||||
end
|
||||
@ -589,4 +589,18 @@ class TestConstStubbing < ActiveSupport::TestCase
|
||||
stub_const(SubclassOfConstStubbable, :CONSTANT, 1) { }
|
||||
end
|
||||
end
|
||||
|
||||
test "stubbing a constant that does not exist can be done with `exists: false`" do
|
||||
stub_const(ConstStubbable, :NOT_A_CONSTANT, 1, exists: false) do
|
||||
assert_equal 1, ConstStubbable::NOT_A_CONSTANT
|
||||
end
|
||||
|
||||
assert_raises(NameError) do
|
||||
ConstStubbable::NOT_A_CONSTANT
|
||||
end
|
||||
|
||||
assert_raises(NameError) do
|
||||
stub_const(Object, :ConstStubbable, 1, exists: false)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
Loading…
Reference in New Issue
Block a user