From b5fa98cc45d11fd856636f5b9d4b60e9c807657b Mon Sep 17 00:00:00 2001 From: zackaj Date: Tue, 17 Dec 2024 23:10:41 +0100 Subject: [PATCH 01/15] added the test --- tests/Integration/Queue/UniqueJobTest.php | 62 +++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 36eb9aeb5cb7..d08da31ba444 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -8,9 +8,15 @@ use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; +use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Bus; +use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Queue; use Orchestra\Testbench\Attributes\WithMigration; #[WithMigration] @@ -130,6 +136,25 @@ public function testLockCanBeReleasedBeforeProcessing() $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); } + public function testLockIsReleasedOnModelNotFoundException() + { + UniqueTestFailJobWithSerializedModels::$handled = false; + $user = User::factoryCreate(); + $user->delete(); + $this->expectException(ModelNotFoundException::class); + + try { + dispatch($job = (new UniqueTestFailJobWithSerializedModels($user))); + $this->runQueueWorkerCommand(['--once' => true]); + Queue::assertPushed(UniqueTestFailJobWithSerializedModels::class, 1); + } finally { + + $this->assertFalse($job::$handled); + $this->assertModelMissing($user); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + } + } + protected function getLockKey($job) { return 'laravel_unique_job:'.(is_string($job) ? $job : get_class($job)).':'; @@ -185,3 +210,40 @@ class UniqueUntilStartTestJob extends UniqueTestJob implements ShouldBeUniqueUnt { public $tries = 2; } + +class UniqueTestFailJobWithSerializedModels implements ShouldBeUnique, ShouldQueue +{ + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + + public $tries = 1; + + public static $handled = false; + + public User $user; + + public function __construct(User $user) + { + $this->user = $user; + } + + public function handle() + { + static::$handled = true; + } +} + +class User extends Model +{ + use HasFactory; + + protected $guarded = []; + + public static function factoryCreate() + { + return self::create([ + 'name' => 'test user', + 'email' => 'testUser@test.com', + 'password' => Hash::make('password'), + ]); + } +} From 3279ede085471d8a7544ec7ffc9c281c530ea06e Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 14:07:29 +0100 Subject: [PATCH 02/15] rewrote the test --- tests/Integration/Queue/UniqueJobTest.php | 33 ++++++----------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index d08da31ba444..637dff530dc9 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -8,16 +8,14 @@ use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; use Illuminate\Contracts\Queue\ShouldQueue; -use Illuminate\Database\Eloquent\Factories\HasFactory; -use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Bus; -use Illuminate\Support\Facades\Hash; -use Illuminate\Support\Facades\Queue; use Orchestra\Testbench\Attributes\WithMigration; +use Orchestra\Testbench\Factories\UserFactory; +use Illuminate\Foundation\Auth\User; #[WithMigration] #[WithMigration('cache')] @@ -139,14 +137,15 @@ public function testLockCanBeReleasedBeforeProcessing() public function testLockIsReleasedOnModelNotFoundException() { UniqueTestFailJobWithSerializedModels::$handled = false; - $user = User::factoryCreate(); - $user->delete(); + /** @var \Illuminate\Foundation\Auth\User */ + $user = UserFactory::new()->create(); + $job = new UniqueTestFailJobWithSerializedModels($user); $this->expectException(ModelNotFoundException::class); try { - dispatch($job = (new UniqueTestFailJobWithSerializedModels($user))); - $this->runQueueWorkerCommand(['--once' => true]); - Queue::assertPushed(UniqueTestFailJobWithSerializedModels::class, 1); + + $user->delete(); + dispatch($job); } finally { $this->assertFalse($job::$handled); @@ -231,19 +230,3 @@ public function handle() static::$handled = true; } } - -class User extends Model -{ - use HasFactory; - - protected $guarded = []; - - public static function factoryCreate() - { - return self::create([ - 'name' => 'test user', - 'email' => 'testUser@test.com', - 'password' => Hash::make('password'), - ]); - } -} From dcccd173164dacb6f7cb0869a9edc4f94da0df8b Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 16:11:52 +0100 Subject: [PATCH 03/15] release unique job lock without instantiating a job instance using a hidden context wrapper, only when ModelNotFound exception is thrown. --- .../Foundation/Bus/PendingDispatch.php | 10 +++ .../Queue/InteractsWithUniqueJobs.php | 77 +++++++++++++++++++ src/Illuminate/Queue/CallQueuedHandler.php | 20 ++++- 3 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index 361454214d95..9a96b014c003 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -7,9 +7,11 @@ use Illuminate\Contracts\Bus\Dispatcher; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldBeUnique; +use Illuminate\Foundation\Queue\InteractsWithUniqueJobs; class PendingDispatch { + use InteractsWithUniqueJobs; /** * The job. * @@ -207,6 +209,10 @@ public function __call($method, $parameters) */ public function __destruct() { + if($this->hasUniqueJob($this->job)){ + $this->addLockToContext($this->job); + } + if (! $this->shouldDispatch()) { return; } elseif ($this->afterResponse) { @@ -214,5 +220,9 @@ public function __destruct() } else { app(Dispatcher::class)->dispatch($this->job); } + + if($this->hasUniqueJob($this->job)){ + $this->forgetLockFromContext(); + } } } diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php new file mode 100644 index 000000000000..f9684468944f --- /dev/null +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -0,0 +1,77 @@ +addHidden([ + 'lockCacheDriver' => $this->getCacheDriver($job), + 'lockKey' => $this->getKey($job), + ]); + } + + /** + * forget the used lock. + */ + public function forgetLockFromContext(): void + { + context()->forgetHidden(['lockCacheDriver', 'lockKey']); + } + + /** + * Get the used cache driver as string from the config. + * CacheManger will handle invalid drivers. + */ + private function getCacheDriver($job): ?string + { + /** @var \Illuminate\Cache\Repository */ + $cache = method_exists($job, 'uniqueVia') ? + $job->uniqueVia() : + app()->make(Repository::class); + + $store = collect(config('cache')['stores']) + + ->firstWhere( + function ($store) use ($cache) { + return $cache === rescue(fn () => cache()->driver(($store['driver']))); + } + ); + + return Arr::get($store, 'driver'); + } + + //NOTE: can I change visibility of the original method in src/Illuminate/Bus/UniqueLock.php ? + /** + * Generate the lock key for the given job. + * + * @param mixed $job + * @return string + */ + private function getKey($job) + { + $uniqueId = method_exists($job, 'uniqueId') + ? $job->uniqueId() + : ($job->uniqueId ?? ''); + + return 'laravel_unique_job:'.get_class($job).':'.$uniqueId; + } +} diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 4f2e9e9ce9ef..a81eeaf292fa 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -207,6 +207,24 @@ protected function ensureUniqueJobLockIsReleased($command) } } + /** + * Ensure the lock for a unique job is released + * when can't instantiate a job instance. + * + * @return void + */ + protected function ensureUniqueJobLockIsReleasedWithoutInstance() + { + /** @var string */ + $driver = context()->getHidden('lockCacheDriver'); + /** @var string */ + $key = context()->getHidden('lockKey'); + + if ($driver && $key) { + cache()->driver($driver)->lock($key)->forceRelease(); + } + } + /** * Handle a model not found exception. * @@ -217,7 +235,7 @@ protected function ensureUniqueJobLockIsReleased($command) protected function handleModelNotFound(Job $job, $e) { $class = $job->resolveName(); - + $this->ensureUniqueJobLockIsReleasedWithoutInstance(); try { $reflectionClass = new ReflectionClass($class); From f22dd958fbe6936c85b2061c55c89537cc6d45b1 Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 16:47:30 +0100 Subject: [PATCH 04/15] refactor: test --- src/Illuminate/Queue/CallQueuedHandler.php | 2 +- tests/Integration/Queue/UniqueJobTest.php | 28 ++++++---------------- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index a81eeaf292fa..f45ba1691117 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -209,7 +209,7 @@ protected function ensureUniqueJobLockIsReleased($command) /** * Ensure the lock for a unique job is released - * when can't instantiate a job instance. + * when can't unserialize the job instance. * * @return void */ diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 637dff530dc9..417398e89010 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -136,18 +136,18 @@ public function testLockCanBeReleasedBeforeProcessing() public function testLockIsReleasedOnModelNotFoundException() { - UniqueTestFailJobWithSerializedModels::$handled = false; + UniqueTestSerializesModelsJob::$handled = false; + /** @var \Illuminate\Foundation\Auth\User */ $user = UserFactory::new()->create(); - $job = new UniqueTestFailJobWithSerializedModels($user); + $job = new UniqueTestSerializesModelsJob($user); + $this->expectException(ModelNotFoundException::class); try { - $user->delete(); dispatch($job); } finally { - $this->assertFalse($job::$handled); $this->assertModelMissing($user); $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); @@ -210,23 +210,9 @@ class UniqueUntilStartTestJob extends UniqueTestJob implements ShouldBeUniqueUnt public $tries = 2; } -class UniqueTestFailJobWithSerializedModels implements ShouldBeUnique, ShouldQueue +class UniqueTestSerializesModelsJob extends UniqueTestJob { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; - - public $tries = 1; - - public static $handled = false; + use SerializesModels; - public User $user; - - public function __construct(User $user) - { - $this->user = $user; - } - - public function handle() - { - static::$handled = true; - } + public function __construct(public User $user) {} } From 78b07bfdbaa4f0e2cc02cfcc4c517aab2d6d84bf Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 16:55:03 +0100 Subject: [PATCH 05/15] refactor: changed the line for ensureUniqueJobLockIsReleasedWithoutInstance() --- src/Illuminate/Queue/CallQueuedHandler.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index f45ba1691117..c0858078a47d 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -235,7 +235,7 @@ protected function ensureUniqueJobLockIsReleasedWithoutInstance() protected function handleModelNotFound(Job $job, $e) { $class = $job->resolveName(); - $this->ensureUniqueJobLockIsReleasedWithoutInstance(); + try { $reflectionClass = new ReflectionClass($class); @@ -249,6 +249,8 @@ protected function handleModelNotFound(Job $job, $e) return $job->delete(); } + $this->ensureUniqueJobLockIsReleasedWithoutInstance(); + return $job->fail($e); } From 6759ee94eea500197e24cdf3dad24cd1c9271711 Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 17:16:26 +0100 Subject: [PATCH 06/15] refactor: changed the name of context keys to a framework specific one --- .../Foundation/Queue/InteractsWithUniqueJobs.php | 8 ++++---- src/Illuminate/Queue/CallQueuedHandler.php | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index f9684468944f..68f61643ff57 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -24,8 +24,8 @@ public function hasUniqueJob($job): bool public function addLockToContext($job) { context()->addHidden([ - 'lockCacheDriver' => $this->getCacheDriver($job), - 'lockKey' => $this->getKey($job), + 'laravel_unique_job_cache_driver' => $this->getCacheDriver($job), + 'laravel_unique_job_key' => $this->getKey($job), ]); } @@ -34,11 +34,11 @@ public function addLockToContext($job) */ public function forgetLockFromContext(): void { - context()->forgetHidden(['lockCacheDriver', 'lockKey']); + context()->forgetHidden(['laravel_unique_job_cache_driver', 'laravel_unique_job_key']); } /** - * Get the used cache driver as string from the config. + * Get the used cache driver as string from the config, * CacheManger will handle invalid drivers. */ private function getCacheDriver($job): ?string diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index c0858078a47d..07c298ecae27 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -216,9 +216,9 @@ protected function ensureUniqueJobLockIsReleased($command) protected function ensureUniqueJobLockIsReleasedWithoutInstance() { /** @var string */ - $driver = context()->getHidden('lockCacheDriver'); + $driver = context()->getHidden('laravel_unique_job_cache_driver'); /** @var string */ - $key = context()->getHidden('lockKey'); + $key = context()->getHidden('laravel_unique_job_key'); if ($driver && $key) { cache()->driver($driver)->lock($key)->forceRelease(); From 13727088b776da27e67bf21ff2be5ffec474fb09 Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 20:39:23 +0100 Subject: [PATCH 07/15] refactor: oneliners --- .../Foundation/Bus/PendingDispatch.php | 10 +++--- .../Queue/InteractsWithUniqueJobs.php | 32 +++++++++++-------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index 9a96b014c003..e1a2e7dca397 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -209,11 +209,11 @@ public function __call($method, $parameters) */ public function __destruct() { - if($this->hasUniqueJob($this->job)){ - $this->addLockToContext($this->job); - } + $this->rememberLockIfJobIsUnique($this->job); if (! $this->shouldDispatch()) { + $this->forgetLockIfJobIsUnique($this->job); + return; } elseif ($this->afterResponse) { app(Dispatcher::class)->dispatchAfterResponse($this->job); @@ -221,8 +221,6 @@ public function __destruct() app(Dispatcher::class)->dispatch($this->job); } - if($this->hasUniqueJob($this->job)){ - $this->forgetLockFromContext(); - } + $this->forgetLockIfJobIsUnique($this->job); } } diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index 68f61643ff57..86615966dbc2 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -9,32 +9,36 @@ trait InteractsWithUniqueJobs { /** - * Determine if job has unique lock. + * Saves the used cache driver for the lock and + * the lock key for emergency forceRelease in + * case we can't instantiate a job instance. */ - public function hasUniqueJob($job): bool + public function rememberLockIfJobIsUnique($job): void { - return $job instanceof ShouldBeUnique; + if ($this->isUniqueJob($job)) { + context()->addHidden([ + 'laravel_unique_job_cache_driver' => $this->getCacheDriver($job), + 'laravel_unique_job_key' => $this->getKey($job), + ]); + } } /** - * Saves the used cache driver for the lock and - * the lock key for emergency forceRelease in - * case we can't instantiate a job instance. + * forget the used lock. */ - public function addLockToContext($job) + public function forgetLockIfJobIsUnique($job): void { - context()->addHidden([ - 'laravel_unique_job_cache_driver' => $this->getCacheDriver($job), - 'laravel_unique_job_key' => $this->getKey($job), - ]); + if ($this->isUniqueJob($job)) { + context()->forgetHidden(['laravel_unique_job_cache_driver', 'laravel_unique_job_key']); + } } /** - * forget the used lock. + * Determine if job has unique lock. */ - public function forgetLockFromContext(): void + private function isUniqueJob($job): bool { - context()->forgetHidden(['laravel_unique_job_cache_driver', 'laravel_unique_job_key']); + return $job instanceof ShouldBeUnique; } /** From c6cdc5c2a7a0149ee850131e9befff94768c32d7 Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 22:06:13 +0100 Subject: [PATCH 08/15] refactor: code style --- .../Foundation/Queue/InteractsWithUniqueJobs.php | 2 +- src/Illuminate/Queue/CallQueuedHandler.php | 4 ++-- tests/Integration/Queue/UniqueJobTest.php | 8 +++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index 86615966dbc2..08a843b8eb2b 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -56,7 +56,7 @@ private function getCacheDriver($job): ?string ->firstWhere( function ($store) use ($cache) { - return $cache === rescue(fn () => cache()->driver(($store['driver']))); + return $cache === rescue(fn () => cache()->driver($store['driver'])); } ); diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 07c298ecae27..74c60de7d03f 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -215,9 +215,9 @@ protected function ensureUniqueJobLockIsReleased($command) */ protected function ensureUniqueJobLockIsReleasedWithoutInstance() { - /** @var string */ + /** @var string */ $driver = context()->getHidden('laravel_unique_job_cache_driver'); - /** @var string */ + /** @var string */ $key = context()->getHidden('laravel_unique_job_key'); if ($driver && $key) { diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 417398e89010..0c12aa730f80 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -9,13 +9,13 @@ use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Database\Eloquent\ModelNotFoundException; +use Illuminate\Foundation\Auth\User; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Bus; use Orchestra\Testbench\Attributes\WithMigration; use Orchestra\Testbench\Factories\UserFactory; -use Illuminate\Foundation\Auth\User; #[WithMigration] #[WithMigration('cache')] @@ -138,7 +138,7 @@ public function testLockIsReleasedOnModelNotFoundException() { UniqueTestSerializesModelsJob::$handled = false; - /** @var \Illuminate\Foundation\Auth\User */ + /** @var \Illuminate\Foundation\Auth\User */ $user = UserFactory::new()->create(); $job = new UniqueTestSerializesModelsJob($user); @@ -214,5 +214,7 @@ class UniqueTestSerializesModelsJob extends UniqueTestJob { use SerializesModels; - public function __construct(public User $user) {} + public function __construct(public User $user) + { + } } From b5128f8bc1bab650ff3a81e756e8e2304ed4d274 Mon Sep 17 00:00:00 2001 From: zackaj Date: Sun, 22 Dec 2024 22:30:45 +0100 Subject: [PATCH 09/15] changed test to mock real queues --- tests/Integration/Queue/UniqueJobTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 0c12aa730f80..2212f291ab28 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -146,7 +146,8 @@ public function testLockIsReleasedOnModelNotFoundException() try { $user->delete(); - dispatch($job); + dispatch_sync($job); + unserialize(serialize($job)); } finally { $this->assertFalse($job::$handled); $this->assertModelMissing($user); From 94690b6387e72d9aac5a6bb956165a5592ed7e9a Mon Sep 17 00:00:00 2001 From: zackaj Date: Fri, 27 Dec 2024 23:05:07 +0100 Subject: [PATCH 10/15] test was not working as expected in local, changed the location of the releasing of the lock to be before all returns --- src/Illuminate/Queue/CallQueuedHandler.php | 4 ++-- tests/Integration/Queue/UniqueJobTest.php | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 74c60de7d03f..510ea718a3e5 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -245,12 +245,12 @@ protected function handleModelNotFound(Job $job, $e) $shouldDelete = false; } + $this->ensureUniqueJobLockIsReleasedWithoutInstance(); + if ($shouldDelete) { return $job->delete(); } - $this->ensureUniqueJobLockIsReleasedWithoutInstance(); - return $job->fail($e); } diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 2212f291ab28..b52c7b0599c1 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -146,7 +146,7 @@ public function testLockIsReleasedOnModelNotFoundException() try { $user->delete(); - dispatch_sync($job); + dispatch($job); unserialize(serialize($job)); } finally { $this->assertFalse($job::$handled); @@ -215,6 +215,8 @@ class UniqueTestSerializesModelsJob extends UniqueTestJob { use SerializesModels; + public $deleteWhenMissingModels = true; + public function __construct(public User $user) { } From c2a60a7005f9432841d259b55df39586b00875aa Mon Sep 17 00:00:00 2001 From: zackaj Date: Fri, 27 Dec 2024 23:16:04 +0100 Subject: [PATCH 11/15] adding runQueueWorkerCommand for ci --- tests/Integration/Queue/UniqueJobTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index b52c7b0599c1..82e567282a6f 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -147,6 +147,7 @@ public function testLockIsReleasedOnModelNotFoundException() try { $user->delete(); dispatch($job); + $this->runQueueWorkerCommand(['--once' => true]); unserialize(serialize($job)); } finally { $this->assertFalse($job::$handled); From ed33b518b3bfe03ba82b2801b82fe7444689c962 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Fri, 17 Jan 2025 14:12:29 -0600 Subject: [PATCH 12/15] formatting --- src/Illuminate/Bus/UniqueLock.php | 2 +- .../Foundation/Bus/PendingDispatch.php | 7 +- .../Queue/InteractsWithUniqueJobs.php | 77 ++++++++----------- 3 files changed, 36 insertions(+), 50 deletions(-) diff --git a/src/Illuminate/Bus/UniqueLock.php b/src/Illuminate/Bus/UniqueLock.php index dea12303b719..9a2726e9d8a5 100644 --- a/src/Illuminate/Bus/UniqueLock.php +++ b/src/Illuminate/Bus/UniqueLock.php @@ -64,7 +64,7 @@ public function release($job) * @param mixed $job * @return string */ - protected function getKey($job) + public static function getKey($job) { $uniqueId = method_exists($job, 'uniqueId') ? $job->uniqueId() diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index e1a2e7dca397..4addfe3e0ae3 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -12,6 +12,7 @@ class PendingDispatch { use InteractsWithUniqueJobs; + /** * The job. * @@ -209,10 +210,10 @@ public function __call($method, $parameters) */ public function __destruct() { - $this->rememberLockIfJobIsUnique($this->job); + $this->addUniqueJobInformationToContext($this->job); if (! $this->shouldDispatch()) { - $this->forgetLockIfJobIsUnique($this->job); + $this->removeUniqueJobInformationFromContext($this->job); return; } elseif ($this->afterResponse) { @@ -221,6 +222,6 @@ public function __destruct() app(Dispatcher::class)->dispatch($this->job); } - $this->forgetLockIfJobIsUnique($this->job); + $this->removeUniqueJobInformationFromContext($this->job); } } diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index 08a843b8eb2b..c517dd092b35 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -2,80 +2,65 @@ namespace Illuminate\Foundation\Queue; -use Illuminate\Cache\Repository; +use Illuminate\Bus\UniqueLock; use Illuminate\Contracts\Queue\ShouldBeUnique; -use Illuminate\Support\Arr; +use Illuminate\Support\Facades\Context; trait InteractsWithUniqueJobs { /** - * Saves the used cache driver for the lock and - * the lock key for emergency forceRelease in - * case we can't instantiate a job instance. + * Store unique job information in the context in case we can't resolve the job on the queue side. + * + * @param object $job + * @return void */ - public function rememberLockIfJobIsUnique($job): void + public function addUniqueJobInformationToContext($job): void { if ($this->isUniqueJob($job)) { - context()->addHidden([ - 'laravel_unique_job_cache_driver' => $this->getCacheDriver($job), - 'laravel_unique_job_key' => $this->getKey($job), + Context::addHidden([ + 'laravel_unique_job_cache_driver' => $this->getUniqueJobCacheStore($job), + 'laravel_unique_job_key' => UniqueLock::getKey($job), ]); } } /** - * forget the used lock. + * Remove the unique job information from the context. + * + * @param object $job + * @return void */ - public function forgetLockIfJobIsUnique($job): void + public function removeUniqueJobInformationFromContext($job): void { if ($this->isUniqueJob($job)) { - context()->forgetHidden(['laravel_unique_job_cache_driver', 'laravel_unique_job_key']); + Context::forgetHidden([ + 'laravel_unique_job_cache_driver', + 'laravel_unique_job_key', + ]); } } /** - * Determine if job has unique lock. - */ - private function isUniqueJob($job): bool - { - return $job instanceof ShouldBeUnique; - } - - /** - * Get the used cache driver as string from the config, - * CacheManger will handle invalid drivers. + * Determine the cache store used by the unique job to acquire locks. + * + * @param object $job + * @return string */ - private function getCacheDriver($job): ?string + private function getUniqueJobCacheStore($job): ?string { - /** @var \Illuminate\Cache\Repository */ - $cache = method_exists($job, 'uniqueVia') ? - $job->uniqueVia() : - app()->make(Repository::class); - - $store = collect(config('cache')['stores']) - - ->firstWhere( - function ($store) use ($cache) { - return $cache === rescue(fn () => cache()->driver($store['driver'])); - } - ); - - return Arr::get($store, 'driver'); + return method_exists($job, 'uniqueVia') + ? $job->uniqueVia() + : config('cache.default'); } - //NOTE: can I change visibility of the original method in src/Illuminate/Bus/UniqueLock.php ? /** - * Generate the lock key for the given job. + * Determine if job should be unique. * * @param mixed $job - * @return string + * @return bool */ - private function getKey($job) + private function isUniqueJob($job): bool { - $uniqueId = method_exists($job, 'uniqueId') - ? $job->uniqueId() - : ($job->uniqueId ?? ''); - - return 'laravel_unique_job:'.get_class($job).':'.$uniqueId; + return $job instanceof ShouldBeUnique; } } From 1cc5a578154699e4b9cc6c441be0569d63a611fe Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Fri, 17 Jan 2025 14:13:31 -0600 Subject: [PATCH 13/15] formatting --- .../Queue/InteractsWithUniqueJobs.php | 2 +- src/Illuminate/Queue/CallQueuedHandler.php | 36 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index c517dd092b35..227b6a6b7459 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -54,7 +54,7 @@ private function getUniqueJobCacheStore($job): ?string } /** - * Determine if job should be unique. + * Determine if job uses unique locks. * * @param mixed $job * @return bool diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 510ea718a3e5..87a25a0f3db1 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -207,24 +207,6 @@ protected function ensureUniqueJobLockIsReleased($command) } } - /** - * Ensure the lock for a unique job is released - * when can't unserialize the job instance. - * - * @return void - */ - protected function ensureUniqueJobLockIsReleasedWithoutInstance() - { - /** @var string */ - $driver = context()->getHidden('laravel_unique_job_cache_driver'); - /** @var string */ - $key = context()->getHidden('laravel_unique_job_key'); - - if ($driver && $key) { - cache()->driver($driver)->lock($key)->forceRelease(); - } - } - /** * Handle a model not found exception. * @@ -254,6 +236,24 @@ protected function handleModelNotFound(Job $job, $e) return $job->fail($e); } + /** + * Ensure the lock for a unique job is released + * when can't unserialize the job instance. + * + * @return void + */ + protected function ensureUniqueJobLockIsReleasedWithoutInstance() + { + /** @var string */ + $driver = context()->getHidden('laravel_unique_job_cache_driver'); + /** @var string */ + $key = context()->getHidden('laravel_unique_job_key'); + + if ($driver && $key) { + cache()->driver($driver)->lock($key)->forceRelease(); + } + } + /** * Call the failed method on the job instance. * From e0ee524eea9e0f8cac0c433ce84cc55d18c24f91 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Fri, 17 Jan 2025 14:14:06 -0600 Subject: [PATCH 14/15] formatting --- src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php | 4 ++-- src/Illuminate/Queue/CallQueuedHandler.php | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index 227b6a6b7459..067a0a433073 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -18,7 +18,7 @@ public function addUniqueJobInformationToContext($job): void { if ($this->isUniqueJob($job)) { Context::addHidden([ - 'laravel_unique_job_cache_driver' => $this->getUniqueJobCacheStore($job), + 'laravel_unique_job_cache_store' => $this->getUniqueJobCacheStore($job), 'laravel_unique_job_key' => UniqueLock::getKey($job), ]); } @@ -34,7 +34,7 @@ public function removeUniqueJobInformationFromContext($job): void { if ($this->isUniqueJob($job)) { Context::forgetHidden([ - 'laravel_unique_job_cache_driver', + 'laravel_unique_job_cache_store', 'laravel_unique_job_key', ]); } diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 87a25a0f3db1..f1a13c40e01d 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -245,12 +245,12 @@ protected function handleModelNotFound(Job $job, $e) protected function ensureUniqueJobLockIsReleasedWithoutInstance() { /** @var string */ - $driver = context()->getHidden('laravel_unique_job_cache_driver'); + $store = context()->getHidden('laravel_unique_job_cache_store'); /** @var string */ $key = context()->getHidden('laravel_unique_job_key'); - if ($driver && $key) { - cache()->driver($driver)->lock($key)->forceRelease(); + if ($store && $key) { + cache()->store($store)->lock($key)->forceRelease(); } } From abeebbed6363116836e673b93a073512caced475 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Fri, 17 Jan 2025 16:51:39 -0600 Subject: [PATCH 15/15] formatting --- .../Queue/InteractsWithUniqueJobs.php | 27 +++++----------- src/Illuminate/Queue/CallQueuedHandler.php | 31 +++++++++++++------ 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php index 067a0a433073..064c3f094233 100644 --- a/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php +++ b/src/Illuminate/Foundation/Queue/InteractsWithUniqueJobs.php @@ -11,12 +11,12 @@ trait InteractsWithUniqueJobs /** * Store unique job information in the context in case we can't resolve the job on the queue side. * - * @param object $job + * @param mixed $job * @return void */ public function addUniqueJobInformationToContext($job): void { - if ($this->isUniqueJob($job)) { + if ($job instanceof ShouldBeUnique) { Context::addHidden([ 'laravel_unique_job_cache_store' => $this->getUniqueJobCacheStore($job), 'laravel_unique_job_key' => UniqueLock::getKey($job), @@ -27,12 +27,12 @@ public function addUniqueJobInformationToContext($job): void /** * Remove the unique job information from the context. * - * @param object $job + * @param mixed $job * @return void */ public function removeUniqueJobInformationFromContext($job): void { - if ($this->isUniqueJob($job)) { + if ($job instanceof ShouldBeUnique) { Context::forgetHidden([ 'laravel_unique_job_cache_store', 'laravel_unique_job_key', @@ -43,24 +43,13 @@ public function removeUniqueJobInformationFromContext($job): void /** * Determine the cache store used by the unique job to acquire locks. * - * @param object $job - * @return string + * @param mixed $job + * @return string|null */ - private function getUniqueJobCacheStore($job): ?string + protected function getUniqueJobCacheStore($job): ?string { return method_exists($job, 'uniqueVia') - ? $job->uniqueVia() + ? $job->uniqueVia()->getName() : config('cache.default'); } - - /** - * Determine if job uses unique locks. - * - * @param mixed $job - * @return bool - */ - private function isUniqueJob($job): bool - { - return $job instanceof ShouldBeUnique; - } } diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index f1a13c40e01d..4bd5aa30feb2 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -6,6 +6,7 @@ use Illuminate\Bus\Batchable; use Illuminate\Bus\UniqueLock; use Illuminate\Contracts\Bus\Dispatcher; +use Illuminate\Contracts\Cache\Factory as CacheFactory; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Container\Container; use Illuminate\Contracts\Encryption\Encrypter; @@ -13,6 +14,7 @@ use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing; use Illuminate\Database\Eloquent\ModelNotFoundException; +use Illuminate\Log\Context\Repository as ContextRepository; use Illuminate\Pipeline\Pipeline; use Illuminate\Queue\Attributes\DeleteWhenMissingModels; use ReflectionClass; @@ -227,7 +229,7 @@ protected function handleModelNotFound(Job $job, $e) $shouldDelete = false; } - $this->ensureUniqueJobLockIsReleasedWithoutInstance(); + $this->ensureUniqueJobLockIsReleasedViaContext(); if ($shouldDelete) { return $job->delete(); @@ -237,20 +239,31 @@ protected function handleModelNotFound(Job $job, $e) } /** - * Ensure the lock for a unique job is released - * when can't unserialize the job instance. + * Ensure the lock for a unique job is released via context. + * + * This is required when we can't unserialize the job due to missing models. * * @return void */ - protected function ensureUniqueJobLockIsReleasedWithoutInstance() + protected function ensureUniqueJobLockIsReleasedViaContext() { - /** @var string */ - $store = context()->getHidden('laravel_unique_job_cache_store'); - /** @var string */ - $key = context()->getHidden('laravel_unique_job_key'); + if (! $this->container->bound(ContextRepository::class) || + ! $this->container->bound(CacheFactory::class)) { + return; + } + + $context = $this->container->make(ContextRepository::class); + + [$store, $key] = [ + $context->getHidden('laravel_unique_job_cache_store'), + $context->getHidden('laravel_unique_job_key'), + ]; if ($store && $key) { - cache()->store($store)->lock($key)->forceRelease(); + $this->container->make(CacheFactory::class) + ->store($store) + ->lock($key) + ->forceRelease(); } }