From 5b81eaa991578aa7184f5c6d64b52def0310b09d Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Sat, 31 Oct 2020 15:04:52 +0530 Subject: [PATCH 1/9] [8.x] Add ability to dispatch unique jobs --- .../Foundation/Bus/PendingDispatch.php | 37 ++++++++++- .../Queue/Middleware/ReleaseUniqueJobLock.php | 28 ++++++++ tests/Integration/Queue/UniqueJobTest.php | 66 +++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php create mode 100644 tests/Integration/Queue/UniqueJobTest.php diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index 74e37b269522..f0bff0a379a8 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -2,7 +2,11 @@ namespace Illuminate\Foundation\Bus; +use Illuminate\Bus\Queueable; +use Illuminate\Container\Container; use Illuminate\Contracts\Bus\Dispatcher; +use Illuminate\Contracts\Cache\Repository as Cache; +use Illuminate\Queue\Middleware\ReleaseUniqueJobLock; class PendingDispatch { @@ -20,6 +24,13 @@ class PendingDispatch */ protected $afterResponse = false; + /** + * Indicates if the job dispatch should be cancelled. + * + * @var bool + */ + protected $cancelDispatch = false; + /** * Create a new pending job dispatch. * @@ -109,6 +120,28 @@ public function chain($chain) return $this; } + /** + * Indicate that the job should be dispatched as a unique job. + * + * @param string $uniqueBy + * @param int $uniqueFor + * @return $this + */ + public function unique($uniqueBy, $uniqueFor = 3600) + { + $lock = Container::getInstance()->make(Cache::class)->lock( + $key = 'unique:'.get_class($this->job).$uniqueBy, $uniqueFor + ); + + $this->cancelDispatch = $this->cancelDispatch || ! $lock->get(); + + if (! $this->cancelDispatch && in_array(Queueable::class, class_uses_recursive($this->job))) { + array_push($this->job->middleware, ReleaseUniqueJobLock::class.':'.$key); + } + + return $this; + } + /** * Indicate that the job should be dispatched after the response is sent to the browser. * @@ -142,7 +175,9 @@ public function __call($method, $parameters) */ public function __destruct() { - if ($this->afterResponse) { + if($this->cancelDispatch) { + // Do nothing. + } elseif ($this->afterResponse) { app(Dispatcher::class)->dispatchAfterResponse($this->job); } else { app(Dispatcher::class)->dispatch($this->job); diff --git a/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php b/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php new file mode 100644 index 000000000000..28bc5fd9477c --- /dev/null +++ b/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php @@ -0,0 +1,28 @@ +make(Cache::class) + ->lock($key) + ->forceRelease(); + } + } +} diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php new file mode 100644 index 000000000000..3862b94d05eb --- /dev/null +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -0,0 +1,66 @@ +unique('test', 60); + Bus::assertDispatched(UniqueTestJob::class); + + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + + Bus::fake(); + UniqueTestJob::dispatch()->unique('test', 60); + Bus::assertNotDispatched(UniqueTestJob::class); + + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + } + + public function testLockIsReleased() + { + UniqueTestJob::$handled = false; + dispatch($job = new UniqueTestJob)->unique('test', 60); + $this->assertTrue($job::$handled); + + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + } + + protected function getLockKey() + { + return 'unique:'.UniqueTestJob::class.'test'; + } +} + +class UniqueTestJob implements ShouldQueue +{ + use InteractsWithQueue, Queueable, Dispatchable; + + public static $handled = false; + + public function handle() + { + static::$handled = true; + } +} \ No newline at end of file From bd5e52a5bea87f0cdb5ae4a0df25396da34c1a67 Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Sat, 31 Oct 2020 15:21:42 +0530 Subject: [PATCH 2/9] [8.x] Add ability to dispatch unique jobs (Option 2) --- src/Illuminate/Contracts/Queue/UniqueJob.php | 8 ++++ .../Foundation/Bus/PendingDispatch.php | 46 ++++++++----------- src/Illuminate/Queue/CallQueuedHandler.php | 25 +++++++++- .../Queue/Middleware/ReleaseUniqueJobLock.php | 28 ----------- tests/Integration/Queue/UniqueJobTest.php | 19 ++++---- 5 files changed, 61 insertions(+), 65 deletions(-) create mode 100644 src/Illuminate/Contracts/Queue/UniqueJob.php delete mode 100644 src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php diff --git a/src/Illuminate/Contracts/Queue/UniqueJob.php b/src/Illuminate/Contracts/Queue/UniqueJob.php new file mode 100644 index 000000000000..603be95f5d42 --- /dev/null +++ b/src/Illuminate/Contracts/Queue/UniqueJob.php @@ -0,0 +1,8 @@ +make(Cache::class)->lock( - $key = 'unique:'.get_class($this->job).$uniqueBy, $uniqueFor - ); - - $this->cancelDispatch = $this->cancelDispatch || ! $lock->get(); - - if (! $this->cancelDispatch && in_array(Queueable::class, class_uses_recursive($this->job))) { - array_push($this->job->middleware, ReleaseUniqueJobLock::class.':'.$key); - } + $this->afterResponse = true; return $this; } /** - * Indicate that the job should be dispatched after the response is sent to the browser. + * Determine if the job should be dispatched. * - * @return $this + * @return bool */ - public function afterResponse() + protected function shouldDispatch() { - $this->afterResponse = true; + if (! ($this->job instanceof UniqueJob)) { + return true; + } - return $this; + $uniqueBy = method_exists($this->job, 'uniqueBy') ? $this->job->uniqueBy() : ''; + + $lock = Container::getInstance()->make(Cache::class)->lock( + $key = 'unique:'.get_class($this->job).$uniqueBy, + property_exists($this->job, 'uniqueFor') ? $this->job->uniqueFor : 0 + ); + + return (bool) $lock->get(); } /** @@ -175,7 +167,7 @@ public function __call($method, $parameters) */ public function __destruct() { - if($this->cancelDispatch) { + if (! $this->shouldDispatch()) { // Do nothing. } elseif ($this->afterResponse) { app(Dispatcher::class)->dispatchAfterResponse($this->job); diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 4a9c9e1f20e4..38d54769480a 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -5,8 +5,10 @@ use Exception; use Illuminate\Bus\Batchable; use Illuminate\Contracts\Bus\Dispatcher; +use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Container\Container; use Illuminate\Contracts\Queue\Job; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Pipeline\Pipeline; use ReflectionClass; @@ -57,7 +59,11 @@ public function call(Job $job, array $data) return $this->handleModelNotFound($job, $e); } - $this->dispatchThroughMiddleware($job, $command); + try { + $this->dispatchThroughMiddleware($job, $command); + } finally { + $this->ensureUniqueJobLockIsReleased($command); + } if (! $job->hasFailed() && ! $job->isReleased()) { $this->ensureNextJobInChainIsDispatched($command); @@ -153,6 +159,23 @@ protected function ensureSuccessfulBatchJobIsRecorded($command) $command->batch()->recordSuccessfulJob($command->job->uuid()); } + /** + * Ensure the lock for the unique job is released. + * + * @param mixed $command + * @return void + */ + protected function ensureUniqueJobLockIsReleased($command) + { + if ($command instanceof UniqueJob) { + $uniqueBy = method_exists($command, 'uniqueBy') ? $command->uniqueBy() : ''; + + $this->container->make(Cache::class) + ->lock('unique:'.get_class($command).$uniqueBy) + ->forceRelease(); + } + } + /** * Handle a model not found exception. * diff --git a/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php b/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php deleted file mode 100644 index 28bc5fd9477c..000000000000 --- a/src/Illuminate/Queue/Middleware/ReleaseUniqueJobLock.php +++ /dev/null @@ -1,28 +0,0 @@ -make(Cache::class) - ->lock($key) - ->forceRelease(); - } - } -} diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 3862b94d05eb..53b005691df7 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -5,6 +5,7 @@ use Illuminate\Bus\Queueable; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Support\Facades\Bus; @@ -26,34 +27,34 @@ protected function tearDown(): void public function testNonUniqueJobsAreDispatched() { Bus::fake(); - UniqueTestJob::dispatch()->unique('test', 60); + UniqueTestJob::dispatch(); Bus::assertDispatched(UniqueTestJob::class); - $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); Bus::fake(); - UniqueTestJob::dispatch()->unique('test', 60); + UniqueTestJob::dispatch(); Bus::assertNotDispatched(UniqueTestJob::class); - $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); } public function testLockIsReleased() { UniqueTestJob::$handled = false; - dispatch($job = new UniqueTestJob)->unique('test', 60); + dispatch($job = new UniqueTestJob); $this->assertTrue($job::$handled); - $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->acquire()); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); } protected function getLockKey() { - return 'unique:'.UniqueTestJob::class.'test'; + return 'unique:'.UniqueTestJob::class; } } -class UniqueTestJob implements ShouldQueue +class UniqueTestJob implements ShouldQueue, UniqueJob { use InteractsWithQueue, Queueable, Dispatchable; @@ -63,4 +64,4 @@ public function handle() { static::$handled = true; } -} \ No newline at end of file +} From 79e9d85173556efc3029bb5e93f424021f1d8bb1 Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Sat, 31 Oct 2020 18:20:30 +0530 Subject: [PATCH 3/9] fix styleci and add UniqueJob interface to job stub --- src/Illuminate/Foundation/Bus/PendingDispatch.php | 1 - src/Illuminate/Foundation/Console/stubs/job.queued.stub | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index e8f0972ba50c..fa2602d0db8e 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -2,7 +2,6 @@ namespace Illuminate\Foundation\Bus; -use Illuminate\Bus\Queueable; use Illuminate\Container\Container; use Illuminate\Contracts\Bus\Dispatcher; use Illuminate\Contracts\Cache\Repository as Cache; diff --git a/src/Illuminate/Foundation/Console/stubs/job.queued.stub b/src/Illuminate/Foundation/Console/stubs/job.queued.stub index 5f0651cb1789..0be12826fb73 100644 --- a/src/Illuminate/Foundation/Console/stubs/job.queued.stub +++ b/src/Illuminate/Foundation/Console/stubs/job.queued.stub @@ -4,6 +4,7 @@ namespace {{ namespace }}; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; From 6ba2618b8c248e2ea6bf915d8fd1474b66f2483e Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 14:45:31 +0530 Subject: [PATCH 4/9] Modify lock release logic --- .../Foundation/Bus/PendingDispatch.php | 8 +- src/Illuminate/Queue/CallQueuedHandler.php | 13 ++- tests/Integration/Queue/UniqueJobTest.php | 104 ++++++++++++++++-- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/src/Illuminate/Foundation/Bus/PendingDispatch.php b/src/Illuminate/Foundation/Bus/PendingDispatch.php index fa2602d0db8e..98c36b3504b4 100644 --- a/src/Illuminate/Foundation/Bus/PendingDispatch.php +++ b/src/Illuminate/Foundation/Bus/PendingDispatch.php @@ -135,11 +135,13 @@ protected function shouldDispatch() return true; } - $uniqueBy = method_exists($this->job, 'uniqueBy') ? $this->job->uniqueBy() : ''; + $uniqueId = method_exists($this->job, 'uniqueId') + ? $this->job->uniqueId() + : ($this->job->uniqueId ?? ''); $lock = Container::getInstance()->make(Cache::class)->lock( - $key = 'unique:'.get_class($this->job).$uniqueBy, - property_exists($this->job, 'uniqueFor') ? $this->job->uniqueFor : 0 + $key = 'unique:'.get_class($this->job).$uniqueId, + $this->job->uniqueFor ?? 0 ); return (bool) $lock->get(); diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index 38d54769480a..ec3bfa48ea27 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -59,9 +59,9 @@ public function call(Job $job, array $data) return $this->handleModelNotFound($job, $e); } - try { - $this->dispatchThroughMiddleware($job, $command); - } finally { + $this->dispatchThroughMiddleware($job, $command); + + if (! $job->isReleased()) { $this->ensureUniqueJobLockIsReleased($command); } @@ -168,10 +168,12 @@ protected function ensureSuccessfulBatchJobIsRecorded($command) protected function ensureUniqueJobLockIsReleased($command) { if ($command instanceof UniqueJob) { - $uniqueBy = method_exists($command, 'uniqueBy') ? $command->uniqueBy() : ''; + $uniqueId = method_exists($command, 'uniqueId') + ? $command->uniqueId() + : ($command->uniqueId ?? ''); $this->container->make(Cache::class) - ->lock('unique:'.get_class($command).$uniqueBy) + ->lock('unique:'.get_class($command).$uniqueId) ->forceRelease(); } } @@ -215,6 +217,7 @@ public function failed(array $data, $e, string $uuid) { $command = unserialize($data['command']); + $this->ensureUniqueJobLockIsReleased($command); $this->ensureFailedBatchJobIsRecorded($uuid, $command, $e); $this->ensureChainCatchCallbacksAreInvoked($uuid, $command, $e); diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 53b005691df7..60a0f593d515 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -6,6 +6,7 @@ use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Contracts\Queue\UniqueJob; +use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Support\Facades\Bus; @@ -17,40 +18,106 @@ */ class UniqueJobTest extends TestCase { + protected function getEnvironmentSetUp($app) + { + $app['config']->set('database.default', 'testbench'); + $app['config']->set('database.connections.testbench', [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => '', + ]); + + $app['db']->connection()->getSchemaBuilder()->create('jobs', function (Blueprint $table) { + $table->bigIncrements('id'); + $table->string('queue'); + $table->longText('payload'); + $table->tinyInteger('attempts')->unsigned(); + $table->unsignedInteger('reserved_at')->nullable(); + $table->unsignedInteger('available_at'); + $table->unsignedInteger('created_at'); + $table->index(['queue', 'reserved_at']); + }); + } + protected function tearDown(): void { + $this->app['db']->connection()->getSchemaBuilder()->drop('jobs'); + parent::tearDown(); m::close(); } - public function testNonUniqueJobsAreDispatched() + public function testUniqueJobsAreNotDispatched() { Bus::fake(); UniqueTestJob::dispatch(); Bus::assertDispatched(UniqueTestJob::class); - $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); + $this->assertFalse( + $this->app->get(Cache::class)->lock($this->getLockKey(UniqueTestJob::class), 10)->get() + ); Bus::fake(); UniqueTestJob::dispatch(); Bus::assertNotDispatched(UniqueTestJob::class); - $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); + $this->assertFalse( + $this->app->get(Cache::class)->lock($this->getLockKey(UniqueTestJob::class), 10)->get() + ); } - public function testLockIsReleased() + public function testLockIsReleasedForSuccessfulJobs() { UniqueTestJob::$handled = false; dispatch($job = new UniqueTestJob); + + $this->assertTrue($job::$handled); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + } + + public function testLockIsReleasedForFailedJobs() + { + UniqueTestFailJob::$handled = false; + + $this->expectException(\Exception::class); + + try { + dispatch($job = new UniqueTestFailJob); + } finally { + $this->assertTrue($job::$handled); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + } + } + + public function testLockIsNotReleasedForJobRetries() + { + UniqueTestRetryJob::$handled = false; + dispatch($job = new UniqueTestRetryJob); + + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + + $this->artisan('queue:work', [ + 'connection' => 'database', + '--once' => true, + ]); + $this->assertTrue($job::$handled); + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + + UniqueTestRetryJob::$handled = false; + $this->artisan('queue:work', [ + 'connection' => 'database', + '--once' => true, + ]); - $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey(), 10)->get()); + $this->assertTrue($job::$handled); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); } - protected function getLockKey() + protected function getLockKey($job) { - return 'unique:'.UniqueTestJob::class; + return 'unique:'.(is_string($job) ? $job : get_class($job)); } } @@ -65,3 +132,26 @@ public function handle() static::$handled = true; } } + +class UniqueTestFailJob implements ShouldQueue, UniqueJob +{ + public $tries = 1; + + use InteractsWithQueue, Queueable, Dispatchable; + + public static $handled = false; + + public function handle() + { + static::$handled = true; + + throw new \Exception; + } +} + +class UniqueTestRetryJob extends UniqueTestFailJob +{ + public $tries = 2; + + public $connection = 'database'; +} From f617ef057565d0f588ab43da6bc2f6ad951768b6 Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 14:48:40 +0530 Subject: [PATCH 5/9] fix styleci --- tests/Integration/Queue/UniqueJobTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 60a0f593d515..02a3197d3497 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -71,7 +71,7 @@ public function testLockIsReleasedForSuccessfulJobs() { UniqueTestJob::$handled = false; dispatch($job = new UniqueTestJob); - + $this->assertTrue($job::$handled); $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); } From 3a8baaee214b9058e0b1a1c64273b4ae67aef310 Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 14:56:33 +0530 Subject: [PATCH 6/9] fix tests --- tests/Integration/Queue/RateLimitedTest.php | 6 +++--- tests/Integration/Queue/RateLimitedWithRedisTest.php | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/Integration/Queue/RateLimitedTest.php b/tests/Integration/Queue/RateLimitedTest.php index c4f12f6144e0..d73dd58a2c29 100644 --- a/tests/Integration/Queue/RateLimitedTest.php +++ b/tests/Integration/Queue/RateLimitedTest.php @@ -88,7 +88,7 @@ protected function assertJobRanSuccessfully($class) $job = m::mock(Job::class); $job->shouldReceive('hasFailed')->once()->andReturn(false); - $job->shouldReceive('isReleased')->once()->andReturn(false); + $job->shouldReceive('isReleased')->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(false); $job->shouldReceive('delete')->once(); @@ -108,7 +108,7 @@ protected function assertJobWasReleased($class) $job->shouldReceive('hasFailed')->once()->andReturn(false); $job->shouldReceive('release')->once(); - $job->shouldReceive('isReleased')->once()->andReturn(true); + $job->shouldReceive('isReleased')->andReturn(true); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(true); $instance->call($job, [ @@ -126,7 +126,7 @@ protected function assertJobWasSkipped($class) $job = m::mock(Job::class); $job->shouldReceive('hasFailed')->once()->andReturn(false); - $job->shouldReceive('isReleased')->once()->andReturn(false); + $job->shouldReceive('isReleased')->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(false); $job->shouldReceive('delete')->once(); diff --git a/tests/Integration/Queue/RateLimitedWithRedisTest.php b/tests/Integration/Queue/RateLimitedWithRedisTest.php index 5956fff06180..175f531bbfa0 100644 --- a/tests/Integration/Queue/RateLimitedWithRedisTest.php +++ b/tests/Integration/Queue/RateLimitedWithRedisTest.php @@ -119,7 +119,7 @@ protected function assertJobRanSuccessfully($testJob) $job = m::mock(Job::class); $job->shouldReceive('hasFailed')->once()->andReturn(false); - $job->shouldReceive('isReleased')->once()->andReturn(false); + $job->shouldReceive('isReleased')->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(false); $job->shouldReceive('delete')->once(); @@ -139,7 +139,7 @@ protected function assertJobWasReleased($testJob) $job->shouldReceive('hasFailed')->once()->andReturn(false); $job->shouldReceive('release')->once(); - $job->shouldReceive('isReleased')->once()->andReturn(true); + $job->shouldReceive('isReleased')->andReturn(true); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(true); $instance->call($job, [ @@ -157,7 +157,7 @@ protected function assertJobWasSkipped($testJob) $job = m::mock(Job::class); $job->shouldReceive('hasFailed')->once()->andReturn(false); - $job->shouldReceive('isReleased')->once()->andReturn(false); + $job->shouldReceive('isReleased')->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(false); $job->shouldReceive('delete')->once(); From e2067d33a002386280c63e559af932cda113d873 Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 15:56:10 +0530 Subject: [PATCH 7/9] Add test for released jobs --- tests/Integration/Queue/UniqueJobTest.php | 43 +++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 02a3197d3497..45c09338ae40 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -115,6 +115,31 @@ public function testLockIsNotReleasedForJobRetries() $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); } + public function testLockIsNotReleasedForJobReleases() + { + UniqueTestReleasedJob::$handled = false; + dispatch($job = new UniqueTestReleasedJob); + + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + + $this->artisan('queue:work', [ + 'connection' => 'database', + '--once' => true, + ]); + + $this->assertTrue($job::$handled); + $this->assertFalse($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + + UniqueTestReleasedJob::$handled = false; + $this->artisan('queue:work', [ + 'connection' => 'database', + '--once' => true, + ]); + + $this->assertFalse($job::$handled); + $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get()); + } + protected function getLockKey($job) { return 'unique:'.(is_string($job) ? $job : get_class($job)); @@ -135,10 +160,10 @@ public function handle() class UniqueTestFailJob implements ShouldQueue, UniqueJob { - public $tries = 1; - use InteractsWithQueue, Queueable, Dispatchable; + public $tries = 1; + public static $handled = false; public function handle() @@ -149,6 +174,20 @@ public function handle() } } +class UniqueTestReleasedJob extends UniqueTestFailJob +{ + public $tries = 1; + + public $connection = 'database'; + + public function handle() + { + static::$handled = true; + + $this->release(); + } +} + class UniqueTestRetryJob extends UniqueTestFailJob { public $tries = 2; From bc45489f4a666221d11875e5fab77cc03d448b8c Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 16:04:25 +0530 Subject: [PATCH 8/9] Remove interface and check method/property instead --- src/Illuminate/Contracts/Queue/UniqueJob.php | 8 -------- src/Illuminate/Foundation/Bus/PendingDispatch.php | 4 ++-- src/Illuminate/Foundation/Console/stubs/job.queued.stub | 1 - src/Illuminate/Queue/CallQueuedHandler.php | 4 ++-- tests/Integration/Queue/UniqueJobTest.php | 9 ++++++--- 5 files changed, 10 insertions(+), 16 deletions(-) delete mode 100644 src/Illuminate/Contracts/Queue/UniqueJob.php diff --git a/src/Illuminate/Contracts/Queue/UniqueJob.php b/src/Illuminate/Contracts/Queue/UniqueJob.php deleted file mode 100644 index 603be95f5d42..000000000000 --- a/src/Illuminate/Contracts/Queue/UniqueJob.php +++ /dev/null @@ -1,8 +0,0 @@ -job instanceof UniqueJob)) { + if (! method_exists($this->job, 'uniqueId') + && ! property_exists($this->job, 'uniqueId')) { return true; } diff --git a/src/Illuminate/Foundation/Console/stubs/job.queued.stub b/src/Illuminate/Foundation/Console/stubs/job.queued.stub index 0be12826fb73..5f0651cb1789 100644 --- a/src/Illuminate/Foundation/Console/stubs/job.queued.stub +++ b/src/Illuminate/Foundation/Console/stubs/job.queued.stub @@ -4,7 +4,6 @@ namespace {{ namespace }}; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; -use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index ec3bfa48ea27..a3a653af25f8 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -8,7 +8,6 @@ use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Container\Container; use Illuminate\Contracts\Queue\Job; -use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Pipeline\Pipeline; use ReflectionClass; @@ -167,7 +166,8 @@ protected function ensureSuccessfulBatchJobIsRecorded($command) */ protected function ensureUniqueJobLockIsReleased($command) { - if ($command instanceof UniqueJob) { + if (method_exists($command, 'uniqueId') + || property_exists($command, 'uniqueId')) { $uniqueId = method_exists($command, 'uniqueId') ? $command->uniqueId() : ($command->uniqueId ?? ''); diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 45c09338ae40..8335e59199f7 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -5,7 +5,6 @@ use Illuminate\Bus\Queueable; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldQueue; -use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; @@ -146,24 +145,28 @@ protected function getLockKey($job) } } -class UniqueTestJob implements ShouldQueue, UniqueJob +class UniqueTestJob implements ShouldQueue { use InteractsWithQueue, Queueable, Dispatchable; public static $handled = false; + public $uniqueId = ''; + public function handle() { static::$handled = true; } } -class UniqueTestFailJob implements ShouldQueue, UniqueJob +class UniqueTestFailJob implements ShouldQueue { use InteractsWithQueue, Queueable, Dispatchable; public $tries = 1; + public $uniqueId = ''; + public static $handled = false; public function handle() From 7c5a18cb6497bc2f2c07e60fa26eb914722a304c Mon Sep 17 00:00:00 2001 From: Paras Malhotra Date: Tue, 3 Nov 2020 21:18:19 +0530 Subject: [PATCH 9/9] Revert "Remove interface and check method/property instead" This reverts commit bc45489f4a666221d11875e5fab77cc03d448b8c. --- src/Illuminate/Contracts/Queue/UniqueJob.php | 8 ++++++++ src/Illuminate/Foundation/Bus/PendingDispatch.php | 4 ++-- src/Illuminate/Foundation/Console/stubs/job.queued.stub | 1 + src/Illuminate/Queue/CallQueuedHandler.php | 4 ++-- tests/Integration/Queue/UniqueJobTest.php | 9 +++------ 5 files changed, 16 insertions(+), 10 deletions(-) create mode 100644 src/Illuminate/Contracts/Queue/UniqueJob.php diff --git a/src/Illuminate/Contracts/Queue/UniqueJob.php b/src/Illuminate/Contracts/Queue/UniqueJob.php new file mode 100644 index 000000000000..603be95f5d42 --- /dev/null +++ b/src/Illuminate/Contracts/Queue/UniqueJob.php @@ -0,0 +1,8 @@ +job, 'uniqueId') - && ! property_exists($this->job, 'uniqueId')) { + if (! ($this->job instanceof UniqueJob)) { return true; } diff --git a/src/Illuminate/Foundation/Console/stubs/job.queued.stub b/src/Illuminate/Foundation/Console/stubs/job.queued.stub index 5f0651cb1789..0be12826fb73 100644 --- a/src/Illuminate/Foundation/Console/stubs/job.queued.stub +++ b/src/Illuminate/Foundation/Console/stubs/job.queued.stub @@ -4,6 +4,7 @@ namespace {{ namespace }}; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php index a3a653af25f8..ec3bfa48ea27 100644 --- a/src/Illuminate/Queue/CallQueuedHandler.php +++ b/src/Illuminate/Queue/CallQueuedHandler.php @@ -8,6 +8,7 @@ use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Container\Container; use Illuminate\Contracts\Queue\Job; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Pipeline\Pipeline; use ReflectionClass; @@ -166,8 +167,7 @@ protected function ensureSuccessfulBatchJobIsRecorded($command) */ protected function ensureUniqueJobLockIsReleased($command) { - if (method_exists($command, 'uniqueId') - || property_exists($command, 'uniqueId')) { + if ($command instanceof UniqueJob) { $uniqueId = method_exists($command, 'uniqueId') ? $command->uniqueId() : ($command->uniqueId ?? ''); diff --git a/tests/Integration/Queue/UniqueJobTest.php b/tests/Integration/Queue/UniqueJobTest.php index 8335e59199f7..45c09338ae40 100644 --- a/tests/Integration/Queue/UniqueJobTest.php +++ b/tests/Integration/Queue/UniqueJobTest.php @@ -5,6 +5,7 @@ use Illuminate\Bus\Queueable; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Contracts\Queue\UniqueJob; use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; @@ -145,28 +146,24 @@ protected function getLockKey($job) } } -class UniqueTestJob implements ShouldQueue +class UniqueTestJob implements ShouldQueue, UniqueJob { use InteractsWithQueue, Queueable, Dispatchable; public static $handled = false; - public $uniqueId = ''; - public function handle() { static::$handled = true; } } -class UniqueTestFailJob implements ShouldQueue +class UniqueTestFailJob implements ShouldQueue, UniqueJob { use InteractsWithQueue, Queueable, Dispatchable; public $tries = 1; - public $uniqueId = ''; - public static $handled = false; public function handle()