From b5fa98cc45d11fd856636f5b9d4b60e9c807657b Mon Sep 17 00:00:00 2001 From: zackaj Date: Tue, 17 Dec 2024 23:10:41 +0100 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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();