From b6cc4d04d755e5025b2d388ce743b73d36bb331b Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 09:26:42 -0400 Subject: [PATCH 1/9] failing test --- .../Integration/Queue/JobDispatchingTest.php | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/Integration/Queue/JobDispatchingTest.php b/tests/Integration/Queue/JobDispatchingTest.php index 148246208def..0d63cb2fbef2 100644 --- a/tests/Integration/Queue/JobDispatchingTest.php +++ b/tests/Integration/Queue/JobDispatchingTest.php @@ -3,8 +3,12 @@ namespace Illuminate\Tests\Integration\Queue; use Illuminate\Bus\Queueable; +use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; +use Illuminate\Support\Facades\Bus; +use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Reflector; use Orchestra\Testbench\TestCase; class JobDispatchingTest extends TestCase @@ -70,6 +74,41 @@ public function testDoesNotDispatchConditionallyWithClosure() $this->assertTrue(Job::$ran); } + + public function testUniqueJobLockIsReleasedForJobDispatchedAfterResponse() + { + // get initial terminatingCallbacks + $terminatingCallbacksReflectionProperty = (new \ReflectionObject($this->app))->getProperty('terminatingCallbacks'); + $terminatingCallbacksReflectionProperty->setAccessible(true); + $startTerminatingCallbacks = $terminatingCallbacksReflectionProperty->getValue($this->app); + + UniqueJob::dispatch('test')->afterResponse(); + $this->assertFalse( + $this->getJobLock(UniqueJob::class, 'test') + ); + + $this->app->terminate(); + $this->assertTrue(UniqueJob::$ran); + // reset terminating callbacks + $terminatingCallbacksReflectionProperty->setValue($this->app, $startTerminatingCallbacks); + + UniqueJob::$ran = false; + UniqueJob::dispatch('test')->afterResponse(); + $this->app->terminate(); + $this->assertTrue(UniqueJob::$ran); + } + + /** + * Helpers + */ + protected function getLockKey($job, $value = null) + { + return ; + } + + private function getJobLock($job, $value = null) { + return $this->app->get(\Illuminate\Contracts\Cache\Repository::class)->lock('laravel_unique_job:' . $job . $value, 10)->get(); + } } class Job implements ShouldQueue @@ -96,3 +135,10 @@ public function replaceValue($value) static::$value = $value; } } + +class UniqueJob extends Job implements ShouldBeUnique +{ + public function uniqueId() { + return self::$value; + } +} From 3ca93b51fc4f0871e2af96365cbf70e3d2d39ce6 Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 09:37:29 -0400 Subject: [PATCH 2/9] rely on PendingDispatch@afterResponse so that ShouldBeUnique is checked --- src/Illuminate/Foundation/Bus/Dispatchable.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Foundation/Bus/Dispatchable.php b/src/Illuminate/Foundation/Bus/Dispatchable.php index 83f19eea5764..ad471bf87fec 100644 --- a/src/Illuminate/Foundation/Bus/Dispatchable.php +++ b/src/Illuminate/Foundation/Bus/Dispatchable.php @@ -96,7 +96,7 @@ public static function dispatchNow(...$arguments) */ public static function dispatchAfterResponse(...$arguments) { - return app(Dispatcher::class)->dispatchAfterResponse(new static(...$arguments)); + return self::dispatch(...$arguments)->afterResponse(); } /** From a748d3c0bb1e94fdef1d9111724e33b625396c5e Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 10:17:16 -0400 Subject: [PATCH 3/9] modifications to failing test --- .../Integration/Queue/JobDispatchingTest.php | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/tests/Integration/Queue/JobDispatchingTest.php b/tests/Integration/Queue/JobDispatchingTest.php index 0d63cb2fbef2..6a0a59960931 100644 --- a/tests/Integration/Queue/JobDispatchingTest.php +++ b/tests/Integration/Queue/JobDispatchingTest.php @@ -3,12 +3,11 @@ namespace Illuminate\Tests\Integration\Queue; use Illuminate\Bus\Queueable; +use Illuminate\Contracts\Cache\Repository; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; -use Illuminate\Support\Facades\Bus; -use Illuminate\Support\Facades\Cache; -use Illuminate\Support\Reflector; +use Illuminate\Queue\InteractsWithQueue; use Orchestra\Testbench\TestCase; class JobDispatchingTest extends TestCase @@ -89,25 +88,30 @@ public function testUniqueJobLockIsReleasedForJobDispatchedAfterResponse() $this->app->terminate(); $this->assertTrue(UniqueJob::$ran); - // reset terminating callbacks + $terminatingCallbacksReflectionProperty->setValue($this->app, $startTerminatingCallbacks); UniqueJob::$ran = false; UniqueJob::dispatch('test')->afterResponse(); $this->app->terminate(); $this->assertTrue(UniqueJob::$ran); + + // acquire job lock and confirm that job is not dispatched after response + $this->assertTrue( + $this->getJobLock(UniqueJob::class, 'test') + ); + $terminatingCallbacksReflectionProperty->setValue($this->app, $startTerminatingCallbacks); + UniqueJob::$ran = false; + UniqueJob::dispatch('test')->afterResponse(); + $this->app->terminate(); + $this->assertFalse(UniqueJob::$ran); } /** - * Helpers + * Helpers. */ - protected function getLockKey($job, $value = null) - { - return ; - } - private function getJobLock($job, $value = null) { - return $this->app->get(\Illuminate\Contracts\Cache\Repository::class)->lock('laravel_unique_job:' . $job . $value, 10)->get(); + return $this->app->get(Repository::class)->lock('laravel_unique_job:' . $job . $value, 10)->get(); } } @@ -138,6 +142,8 @@ public function replaceValue($value) class UniqueJob extends Job implements ShouldBeUnique { + use InteractsWithQueue; + public function uniqueId() { return self::$value; } From 05be341a99509a2f0c070564986f1a8bb024ce95 Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 10:20:31 -0400 Subject: [PATCH 4/9] move lock/middleware handling to CallQueuedHandler@handle() --- src/Illuminate/Queue/CallQueuedHandler.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 22fe9ccab89a..c3de828c9dcf 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -63,6 +63,20 @@ public function call(Job $job, array $data) return $this->handleModelNotFound($job, $e); } + $this->handle($command, $job); + } + + /** + * Handle job locks, dispatching the job through middleware, and releasing the job. + * + * @param mixed $command + * @param Job|null $job + * @return void + */ + public function handle($command, ?Job $job = null) + { + $job ??= $command->job; + if ($command instanceof ShouldBeUniqueUntilProcessing) { $this->ensureUniqueJobLockIsReleased($command); } From ba0aeac561024a13f6d25346c48e8efbd1fb01a1 Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 10:21:13 -0400 Subject: [PATCH 5/9] use CallQueuedHandler@handle if job employs InteractsWithQueue --- src/Illuminate/Foundation/Bus/PendingDispatch.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index 1e514e5b0b78..f0433503fea4 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -7,6 +7,8 @@ use Illuminate\Contracts\Bus\Dispatcher; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldBeUnique; +use Illuminate\Queue\CallQueuedHandler; +use Illuminate\Queue\InteractsWithQueue; class PendingDispatch { @@ -188,7 +190,12 @@ public function __destruct() if (! $this->shouldDispatch()) { return; } elseif ($this->afterResponse) { - app(Dispatcher::class)->dispatchAfterResponse($this->job); + app(Dispatcher::class)->dispatchAfterResponse( + $this->job, + in_array(InteractsWithQueue::class, class_uses_recursive($this->job)) + ? app(CallQueuedHandler::class) + : null + ); } else { app(Dispatcher::class)->dispatch($this->job); } From 14afb26320df2c5f2148d387decd6d0052af842e Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 10:22:04 -0400 Subject: [PATCH 6/9] style --- tests/Integration/Queue/JobDispatchingTest.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/Integration/Queue/JobDispatchingTest.php b/tests/Integration/Queue/JobDispatchingTest.php index 6a0a59960931..dd7043a7f135 100644 --- a/tests/Integration/Queue/JobDispatchingTest.php +++ b/tests/Integration/Queue/JobDispatchingTest.php @@ -110,8 +110,9 @@ public function testUniqueJobLockIsReleasedForJobDispatchedAfterResponse() /** * Helpers. */ - private function getJobLock($job, $value = null) { - return $this->app->get(Repository::class)->lock('laravel_unique_job:' . $job . $value, 10)->get(); + private function getJobLock($job, $value = null) + { + return $this->app->get(Repository::class)->lock('laravel_unique_job:'.$job.$value, 10)->get(); } } @@ -144,7 +145,8 @@ class UniqueJob extends Job implements ShouldBeUnique { use InteractsWithQueue; - public function uniqueId() { + public function uniqueId() + { return self::$value; } } From 7d1af1073a1f21d31ce7dd178d4afc713f5245a1 Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 16:14:56 -0400 Subject: [PATCH 7/9] revert changes to CallQueuedHandler & PendingDispatch --- src/Illuminate/Foundation/Bus/PendingDispatch.php | 9 +-------- src/Illuminate/Queue/CallQueuedHandler.php | 14 -------------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index f0433503fea4..1e514e5b0b78 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -7,8 +7,6 @@ use Illuminate\Contracts\Bus\Dispatcher; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldBeUnique; -use Illuminate\Queue\CallQueuedHandler; -use Illuminate\Queue\InteractsWithQueue; class PendingDispatch { @@ -190,12 +188,7 @@ public function __destruct() if (! $this->shouldDispatch()) { return; } elseif ($this->afterResponse) { - app(Dispatcher::class)->dispatchAfterResponse( - $this->job, - in_array(InteractsWithQueue::class, class_uses_recursive($this->job)) - ? app(CallQueuedHandler::class) - : null - ); + app(Dispatcher::class)->dispatchAfterResponse($this->job); } else { app(Dispatcher::class)->dispatch($this->job); } diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index c3de828c9dcf..22fe9ccab89a 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -63,20 +63,6 @@ public function call(Job $job, array $data) return $this->handleModelNotFound($job, $e); } - $this->handle($command, $job); - } - - /** - * Handle job locks, dispatching the job through middleware, and releasing the job. - * - * @param mixed $command - * @param Job|null $job - * @return void - */ - public function handle($command, ?Job $job = null) - { - $job ??= $command->job; - if ($command instanceof ShouldBeUniqueUntilProcessing) { $this->ensureUniqueJobLockIsReleased($command); } From 97078520d89c9f80e8a09415181a4e1b40a7cc46 Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 16:15:51 -0400 Subject: [PATCH 8/9] switch Bus\Dispatcher@dispatchAfterResponse to rely on Dispatcher@dispatchSync() --- src/Illuminate/Bus/Dispatcher.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Bus/Dispatcher.php b/src/Illuminate/Bus/Dispatcher.php index 4dc390e653fb..8ed3a21b7c4f 100644 --- a/src/Illuminate/Bus/Dispatcher.php +++ b/src/Illuminate/Bus/Dispatcher.php @@ -263,7 +263,7 @@ protected function pushCommandToQueue($queue, $command) public function dispatchAfterResponse($command, $handler = null) { $this->container->terminating(function () use ($command, $handler) { - $this->dispatchNow($command, $handler); + $this->dispatchSync($command, $handler); }); } From c671e1ae3835149fac1afe4c0ba72ca6d0f2f00d Mon Sep 17 00:00:00 2001 From: Luke Kuzmish Date: Mon, 17 Apr 2023 16:23:06 -0400 Subject: [PATCH 9/9] add `dispatchAfterResponse` test --- tests/Integration/Queue/JobDispatchingTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/Integration/Queue/JobDispatchingTest.php b/tests/Integration/Queue/JobDispatchingTest.php index dd7043a7f135..f7fe354180a8 100644 --- a/tests/Integration/Queue/JobDispatchingTest.php +++ b/tests/Integration/Queue/JobDispatchingTest.php @@ -81,7 +81,7 @@ public function testUniqueJobLockIsReleasedForJobDispatchedAfterResponse() $terminatingCallbacksReflectionProperty->setAccessible(true); $startTerminatingCallbacks = $terminatingCallbacksReflectionProperty->getValue($this->app); - UniqueJob::dispatch('test')->afterResponse(); + UniqueJob::dispatchAfterResponse('test'); $this->assertFalse( $this->getJobLock(UniqueJob::class, 'test') ); @@ -105,6 +105,11 @@ public function testUniqueJobLockIsReleasedForJobDispatchedAfterResponse() UniqueJob::dispatch('test')->afterResponse(); $this->app->terminate(); $this->assertFalse(UniqueJob::$ran); + + // confirm that dispatchAfterResponse also does not run + UniqueJob::dispatchAfterResponse('test'); + $this->app->terminate(); + $this->assertFalse(UniqueJob::$ran); } /**