Skip to content

Commit 66eeb82

Browse files
zackAJtaylorotwell
andauthored
[11.x] Fix unique job lock is not released on model not found exception, lock gets stuck. (#54000)
* added the test * rewrote the test * release unique job lock without instantiating a job instance using a hidden context wrapper, only when ModelNotFound exception is thrown. * refactor: test * refactor: changed the line for ensureUniqueJobLockIsReleasedWithoutInstance() * refactor: changed the name of context keys to a framework specific one * refactor: oneliners * refactor: code style * changed test to mock real queues * test was not working as expected in local, changed the location of the releasing of the lock to be before all returns * adding runQueueWorkerCommand for ci * formatting * formatting * formatting * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
1 parent aa01ced commit 66eeb82

File tree

5 files changed

+135
-1
lines changed

5 files changed

+135
-1
lines changed

src/Illuminate/Bus/UniqueLock.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public function release($job)
6464
* @param mixed $job
6565
* @return string
6666
*/
67-
protected function getKey($job)
67+
public static function getKey($job)
6868
{
6969
$uniqueId = method_exists($job, 'uniqueId')
7070
? $job->uniqueId()

src/Illuminate/Foundation/Bus/PendingDispatch.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,12 @@
77
use Illuminate\Contracts\Bus\Dispatcher;
88
use Illuminate\Contracts\Cache\Repository as Cache;
99
use Illuminate\Contracts\Queue\ShouldBeUnique;
10+
use Illuminate\Foundation\Queue\InteractsWithUniqueJobs;
1011

1112
class PendingDispatch
1213
{
14+
use InteractsWithUniqueJobs;
15+
1316
/**
1417
* The job.
1518
*
@@ -207,12 +210,18 @@ public function __call($method, $parameters)
207210
*/
208211
public function __destruct()
209212
{
213+
$this->addUniqueJobInformationToContext($this->job);
214+
210215
if (! $this->shouldDispatch()) {
216+
$this->removeUniqueJobInformationFromContext($this->job);
217+
211218
return;
212219
} elseif ($this->afterResponse) {
213220
app(Dispatcher::class)->dispatchAfterResponse($this->job);
214221
} else {
215222
app(Dispatcher::class)->dispatch($this->job);
216223
}
224+
225+
$this->removeUniqueJobInformationFromContext($this->job);
217226
}
218227
}
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<?php
2+
3+
namespace Illuminate\Foundation\Queue;
4+
5+
use Illuminate\Bus\UniqueLock;
6+
use Illuminate\Contracts\Queue\ShouldBeUnique;
7+
use Illuminate\Support\Facades\Context;
8+
9+
trait InteractsWithUniqueJobs
10+
{
11+
/**
12+
* Store unique job information in the context in case we can't resolve the job on the queue side.
13+
*
14+
* @param mixed $job
15+
* @return void
16+
*/
17+
public function addUniqueJobInformationToContext($job): void
18+
{
19+
if ($job instanceof ShouldBeUnique) {
20+
Context::addHidden([
21+
'laravel_unique_job_cache_store' => $this->getUniqueJobCacheStore($job),
22+
'laravel_unique_job_key' => UniqueLock::getKey($job),
23+
]);
24+
}
25+
}
26+
27+
/**
28+
* Remove the unique job information from the context.
29+
*
30+
* @param mixed $job
31+
* @return void
32+
*/
33+
public function removeUniqueJobInformationFromContext($job): void
34+
{
35+
if ($job instanceof ShouldBeUnique) {
36+
Context::forgetHidden([
37+
'laravel_unique_job_cache_store',
38+
'laravel_unique_job_key',
39+
]);
40+
}
41+
}
42+
43+
/**
44+
* Determine the cache store used by the unique job to acquire locks.
45+
*
46+
* @param mixed $job
47+
* @return string|null
48+
*/
49+
protected function getUniqueJobCacheStore($job): ?string
50+
{
51+
return method_exists($job, 'uniqueVia')
52+
? $job->uniqueVia()->getName()
53+
: config('cache.default');
54+
}
55+
}

src/Illuminate/Queue/CallQueuedHandler.php

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@
66
use Illuminate\Bus\Batchable;
77
use Illuminate\Bus\UniqueLock;
88
use Illuminate\Contracts\Bus\Dispatcher;
9+
use Illuminate\Contracts\Cache\Factory as CacheFactory;
910
use Illuminate\Contracts\Cache\Repository as Cache;
1011
use Illuminate\Contracts\Container\Container;
1112
use Illuminate\Contracts\Encryption\Encrypter;
1213
use Illuminate\Contracts\Queue\Job;
1314
use Illuminate\Contracts\Queue\ShouldBeUnique;
1415
use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
1516
use Illuminate\Database\Eloquent\ModelNotFoundException;
17+
use Illuminate\Log\Context\Repository as ContextRepository;
1618
use Illuminate\Pipeline\Pipeline;
1719
use Illuminate\Queue\Attributes\DeleteWhenMissingModels;
1820
use ReflectionClass;
@@ -227,13 +229,44 @@ protected function handleModelNotFound(Job $job, $e)
227229
$shouldDelete = false;
228230
}
229231

232+
$this->ensureUniqueJobLockIsReleasedViaContext();
233+
230234
if ($shouldDelete) {
231235
return $job->delete();
232236
}
233237

234238
return $job->fail($e);
235239
}
236240

241+
/**
242+
* Ensure the lock for a unique job is released via context.
243+
*
244+
* This is required when we can't unserialize the job due to missing models.
245+
*
246+
* @return void
247+
*/
248+
protected function ensureUniqueJobLockIsReleasedViaContext()
249+
{
250+
if (! $this->container->bound(ContextRepository::class) ||
251+
! $this->container->bound(CacheFactory::class)) {
252+
return;
253+
}
254+
255+
$context = $this->container->make(ContextRepository::class);
256+
257+
[$store, $key] = [
258+
$context->getHidden('laravel_unique_job_cache_store'),
259+
$context->getHidden('laravel_unique_job_key'),
260+
];
261+
262+
if ($store && $key) {
263+
$this->container->make(CacheFactory::class)
264+
->store($store)
265+
->lock($key)
266+
->forceRelease();
267+
}
268+
}
269+
237270
/**
238271
* Call the failed method on the job instance.
239272
*

tests/Integration/Queue/UniqueJobTest.php

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,14 @@
88
use Illuminate\Contracts\Queue\ShouldBeUnique;
99
use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
1010
use Illuminate\Contracts\Queue\ShouldQueue;
11+
use Illuminate\Database\Eloquent\ModelNotFoundException;
12+
use Illuminate\Foundation\Auth\User;
1113
use Illuminate\Foundation\Bus\Dispatchable;
1214
use Illuminate\Queue\InteractsWithQueue;
15+
use Illuminate\Queue\SerializesModels;
1316
use Illuminate\Support\Facades\Bus;
1417
use Orchestra\Testbench\Attributes\WithMigration;
18+
use Orchestra\Testbench\Factories\UserFactory;
1519

1620
#[WithMigration]
1721
#[WithMigration('cache')]
@@ -130,6 +134,28 @@ public function testLockCanBeReleasedBeforeProcessing()
130134
$this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get());
131135
}
132136

137+
public function testLockIsReleasedOnModelNotFoundException()
138+
{
139+
UniqueTestSerializesModelsJob::$handled = false;
140+
141+
/** @var \Illuminate\Foundation\Auth\User */
142+
$user = UserFactory::new()->create();
143+
$job = new UniqueTestSerializesModelsJob($user);
144+
145+
$this->expectException(ModelNotFoundException::class);
146+
147+
try {
148+
$user->delete();
149+
dispatch($job);
150+
$this->runQueueWorkerCommand(['--once' => true]);
151+
unserialize(serialize($job));
152+
} finally {
153+
$this->assertFalse($job::$handled);
154+
$this->assertModelMissing($user);
155+
$this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get());
156+
}
157+
}
158+
133159
protected function getLockKey($job)
134160
{
135161
return 'laravel_unique_job:'.(is_string($job) ? $job : get_class($job)).':';
@@ -185,3 +211,14 @@ class UniqueUntilStartTestJob extends UniqueTestJob implements ShouldBeUniqueUnt
185211
{
186212
public $tries = 2;
187213
}
214+
215+
class UniqueTestSerializesModelsJob extends UniqueTestJob
216+
{
217+
use SerializesModels;
218+
219+
public $deleteWhenMissingModels = true;
220+
221+
public function __construct(public User $user)
222+
{
223+
}
224+
}

0 commit comments

Comments
 (0)