-
Notifications
You must be signed in to change notification settings - Fork 11.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inconsistent ShouldBeUnique behavior for missing models #49890
Comments
The right approach here is to not make |
The bug is not about the uniqueId as this can be addressed in the user code. The bug is that Neither of the cases is mentioned in the documentation and it took a while to figure out what was happening on the server and why there are so many stale unique locks. |
To make it more clear, here's another PoC to be executed in the same environment (step 3). <?php
use Illuminate\Bus\Queueable;
use Illuminate\Queue\WorkerOptions;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Facades\Redis;
use Illuminate\Queue\SerializesModels;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Contracts\Console\Kernel;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Contracts\Queue\ShouldBeUnique;
require 'vendor/autoload.php';
$app = require __DIR__.'/bootstrap/app.php';
$cli = $app->make(Kernel::class);
$cli->bootstrap();
config([
'database.default' => 'sqlite',
'queue.default' => 'redis',
'cache.default' => 'redis',
]);
@unlink(config('database.connections.sqlite.database'));
$cli->call('migrate', ['--force' => true]);
DB::unprepared("
CREATE TABLE main (
id INTEGER NOT NULL PRIMARY KEY,
name VARCHAR(255) UNIQUE
)
");
// ensure there are no keys in the DB
Redis::flushdb();
class Main extends Model {
public $table = 'main';
public $timestamps = false;
public $fillable = ['name'];
}
class MyJob implements ShouldQueue, ShouldBeUnique {
use Dispatchable, Queueable, SerializesModels;
public $tries = 1;
public $deleteWhenMissingModels = true;
public function __construct(
public Main $main,
)
{
}
public function handle() {
}
}
// create the job instance
$instance = Main::create(['name' => 'main model']);
// and schedule a job for them
MyJob::dispatch($instance);
// delete the secondary model
$instance->delete();
// run the job
app('queue.worker')->runNextJob('redis', 'default', new WorkerOptions);
echo 'Queue size: ', Queue::size('default'), PHP_EOL; // no tasks left
echo 'Redis contents:', PHP_EOL;
dump(Redis::keys('*')); // unique id is still there I would also argue, that unique ID is being created and maintained by the framework. Hence it is framework's job to ensure that the unique ID is properly attached to the job to avoid special cases like the described one. But skip that for now, the fix for the lock itself would be highly appreciated. |
Thanks @naquad for that explanation. I'll re-open this one. We'd appreciate a PR for a fix if anyone can figure one out. |
Thank you for reporting this issue! As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub. If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team. Thank you! |
Here's my take on the fix that needs some blessing before I get knee-deep into fixing tests. A somewhat hackish way would be the following:
The patch would look like this: diff --git a/src/Illuminate/Bus/UniqueLock.php b/src/Illuminate/Bus/UniqueLock.php
index a4066b77c1..f07830caf3 100644
--- a/src/Illuminate/Bus/UniqueLock.php
+++ b/src/Illuminate/Bus/UniqueLock.php
@@ -3,6 +3,7 @@
namespace Illuminate\Bus;
use Illuminate\Contracts\Cache\Repository as Cache;
+use Illuminate\Support\Str;
class UniqueLock
{
@@ -40,7 +41,7 @@ public function acquire($job)
? $job->uniqueVia()
: $this->cache;
- return (bool) $cache->lock($this->getKey($job), $uniqueFor)->get();
+ return (bool) $cache->lock(static::getKey($job), $uniqueFor)->get();
}
/**
@@ -55,7 +56,7 @@ public function release($job)
? $job->uniqueVia()
: $this->cache;
- $cache->lock($this->getKey($job))->forceRelease();
+ $cache->lock(static::getKey($job))->forceRelease();
}
/**
@@ -64,8 +65,12 @@ public function release($job)
* @param mixed $job
* @return string
*/
- protected function getKey($job)
+ public static function getKey($job)
{
+ if (is_string($job) && Str::startsWith($job, 'laravel_unique_job:')) {
+ return $job;
+ }
+
$uniqueId = method_exists($job, 'uniqueId')
? $job->uniqueId()
: ($job->uniqueId ?? '');
diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php
index 5bee1d9ebb..6a545f43e8 100644
--- a/src/Illuminate/Queue/CallQueuedHandler.php
+++ b/src/Illuminate/Queue/CallQueuedHandler.php
@@ -10,7 +10,6 @@
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Encryption\Encrypter;
use Illuminate\Contracts\Queue\Job;
-use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Pipeline\Pipeline;
@@ -60,17 +59,18 @@ public function call(Job $job, array $data)
$job, $this->getCommand($data)
);
} catch (ModelNotFoundException $e) {
+ $this->ensureUniqueJobLockIsReleased($data);
return $this->handleModelNotFound($job, $e);
}
if ($command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
$this->dispatchThroughMiddleware($job, $command);
if (! $job->isReleased() && ! $command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
if (! $job->hasFailed() && ! $job->isReleased()) {
@@ -196,13 +196,14 @@ protected function ensureSuccessfulBatchJobIsRecorded($command)
/**
* Ensure the lock for a unique job is released.
*
- * @param mixed $command
+ * @param array $data
* @return void
*/
- protected function ensureUniqueJobLockIsReleased($command)
+ protected function ensureUniqueJobLockIsReleased($data)
{
- if ($command instanceof ShouldBeUnique) {
- (new UniqueLock($this->container->make(Cache::class)))->release($command);
+ if (isset($data['uniqueId'])) {
+ (new UniqueLock($this->container->make(Cache::class)))
+ ->release($data['uniqueId']);
}
}
@@ -246,7 +247,7 @@ public function failed(array $data, $e, string $uuid)
$command = $this->getCommand($data);
if (! $command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
if ($command instanceof \__PHP_Incomplete_Class) {
diff --git a/src/Illuminate/Queue/Queue.php b/src/Illuminate/Queue/Queue.php
index 09eb245263..a87c81c5f3 100755
--- a/src/Illuminate/Queue/Queue.php
+++ b/src/Illuminate/Queue/Queue.php
@@ -4,9 +4,12 @@
use Closure;
use DateTimeInterface;
+use Illuminate\Bus\UniqueLock;
use Illuminate\Container\Container;
use Illuminate\Contracts\Encryption\Encrypter;
use Illuminate\Contracts\Queue\ShouldBeEncrypted;
+use Illuminate\Contracts\Queue\ShouldBeUnique;
+use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
use Illuminate\Contracts\Queue\ShouldQueueAfterCommit;
use Illuminate\Queue\Events\JobQueued;
use Illuminate\Queue\Events\JobQueueing;
@@ -150,6 +153,7 @@ protected function createObjectPayload($job, $queue)
'timeout' => $job->timeout ?? null,
'retryUntil' => $this->getJobExpiration($job),
'data' => [
+ 'uniqueId' => $this->getJobUniqueId($job),
'commandName' => $job,
'command' => $job,
],
@@ -167,6 +171,24 @@ protected function createObjectPayload($job, $queue)
]);
}
+ /**
+ * Get the unique ID of the job
+ *
+ * @param object $job
+ * @return string|null
+ */
+ protected function getJobUniqueId($job)
+ {
+ if (
+ $job instanceof ShouldBeUnique ||
+ $job instanceof ShouldBeUniqueUntilProcessing
+ ) {
+ return UniqueLock::getKey($job);
+ }
+
+ return null;
+ }
+
/**
* Get the display name for the given job.
* EDIT: Patch changes. |
Scratch that. |
Ok, I think I got it right. The idea is: to create a helper class to manage the lock based on the initial data. This way the diff --git a/src/Illuminate/Bus/UniqueLock.php b/src/Illuminate/Bus/UniqueLock.php
index a4066b77c1..c70a7b252a 100644
--- a/src/Illuminate/Bus/UniqueLock.php
+++ b/src/Illuminate/Bus/UniqueLock.php
@@ -70,6 +70,10 @@ protected function getKey($job)
? $job->uniqueId()
: ($job->uniqueId ?? '');
- return 'laravel_unique_job:'.get_class($job).$uniqueId;
+ $jobName = property_exists($job, 'jobName')
+ ? $job->jobName
+ : get_class($job);
+
+ return 'laravel_unique_job:'.$jobName.$uniqueId;
}
}
diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php
index 5bee1d9ebb..f5d326e9e3 100644
--- a/src/Illuminate/Queue/CallQueuedHandler.php
+++ b/src/Illuminate/Queue/CallQueuedHandler.php
@@ -4,13 +4,10 @@
use Exception;
use Illuminate\Bus\Batchable;
-use Illuminate\Bus\UniqueLock;
use Illuminate\Contracts\Bus\Dispatcher;
-use Illuminate\Contracts\Cache\Repository as Cache;
use Illuminate\Contracts\Container\Container;
use Illuminate\Contracts\Encryption\Encrypter;
use Illuminate\Contracts\Queue\Job;
-use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Pipeline\Pipeline;
@@ -60,17 +57,18 @@ public function call(Job $job, array $data)
$job, $this->getCommand($data)
);
} catch (ModelNotFoundException $e) {
+ $this->ensureUniqueJobLockIsReleased($data);
return $this->handleModelNotFound($job, $e);
}
if ($command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
$this->dispatchThroughMiddleware($job, $command);
if (! $job->isReleased() && ! $command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
if (! $job->hasFailed() && ! $job->isReleased()) {
@@ -83,6 +81,29 @@ public function call(Job $job, array $data)
}
}
+ /**
+ * Get the unserialized object from the given payload.
+ *
+ * @param string $key
+ * @param array $data
+ * @return mixed
+ */
+ protected function getUnserializedItem(string $key, array $data)
+ {
+ if (isset($data[$key])) {
+ if (str_starts_with($data[$key], 'O:')) {
+ return unserialize($data[$key]);
+ }
+
+ if ($this->container->bound(Encrypter::class)) {
+ return unserialize($this->container[Encrypter::class]
+ ->decrypt($data[$key]));
+ }
+ }
+
+ return null;
+ }
+
/**
* Get the command from the given payload.
*
@@ -93,17 +114,25 @@ public function call(Job $job, array $data)
*/
protected function getCommand(array $data)
{
- if (str_starts_with($data['command'], 'O:')) {
- return unserialize($data['command']);
- }
-
- if ($this->container->bound(Encrypter::class)) {
- return unserialize($this->container[Encrypter::class]->decrypt($data['command']));
+ $command = $this->getUnserializedItem('command', $data);
+ if ($command !== null) {
+ return $command;
}
throw new RuntimeException('Unable to extract job payload.');
}
+ /**
+ * Get the unique handler from the given payload.
+ *
+ * @param array $data
+ * @return \Illuminate\Queue\UniqueHandler|null
+ */
+ protected function getUniqueHandler(array $data)
+ {
+ return $this->getUnserializedItem('uniqueHandler', $data);
+ }
+
/**
* Dispatch the given job / command through its specified middleware.
*
@@ -196,13 +225,14 @@ protected function ensureSuccessfulBatchJobIsRecorded($command)
/**
* Ensure the lock for a unique job is released.
*
- * @param mixed $command
+ * @param array $data
* @return void
*/
- protected function ensureUniqueJobLockIsReleased($command)
+ protected function ensureUniqueJobLockIsReleased($data)
{
- if ($command instanceof ShouldBeUnique) {
- (new UniqueLock($this->container->make(Cache::class)))->release($command);
+ $handler = $this->getUniqueHandler($data);
+ if ($handler !== null) {
+ $handler->withContainer($this->container)->release();
}
}
@@ -246,7 +276,7 @@ public function failed(array $data, $e, string $uuid)
$command = $this->getCommand($data);
if (! $command instanceof ShouldBeUniqueUntilProcessing) {
- $this->ensureUniqueJobLockIsReleased($command);
+ $this->ensureUniqueJobLockIsReleased($data);
}
if ($command instanceof \__PHP_Incomplete_Class) {
diff --git a/src/Illuminate/Queue/Queue.php b/src/Illuminate/Queue/Queue.php
index 09eb245263..95cd2448ee 100755
--- a/src/Illuminate/Queue/Queue.php
+++ b/src/Illuminate/Queue/Queue.php
@@ -139,6 +139,8 @@ protected function createPayloadArray($job, $queue, $data = '')
*/
protected function createObjectPayload($job, $queue)
{
+ $handler = UniqueHandler::forJob($job);
+
$payload = $this->withCreatePayloadHooks($queue, [
'uuid' => (string) Str::uuid(),
'displayName' => $this->getDisplayName($job),
@@ -150,17 +152,27 @@ protected function createObjectPayload($job, $queue)
'timeout' => $job->timeout ?? null,
'retryUntil' => $this->getJobExpiration($job),
'data' => [
+ 'uniqueHandler' => $handler,
'commandName' => $job,
'command' => $job,
],
]);
- $command = $this->jobShouldBeEncrypted($job) && $this->container->bound(Encrypter::class)
- ? $this->container[Encrypter::class]->encrypt(serialize(clone $job))
- : serialize(clone $job);
+ $handler = serialize($handler);
+ $command = serialize($job);
+
+ if (
+ $this->jobShouldBeEncrypted($job) &&
+ $this->container->bound(Encrypter::class)
+ ) {
+ $encrypter = $this->container[Encrypter::class];
+ $handler = $encrypter->encrypt($handler);
+ $command = $encrypter->encrypt($command);
+ }
return array_merge($payload, [
'data' => array_merge($payload['data'], [
+ 'uniqueHandler' => $handler,
'commandName' => get_class($job),
'command' => $command,
]),
diff --git a/src/Illuminate/Queue/UniqueHandler.php b/src/Illuminate/Queue/UniqueHandler.php
new file mode 100644
index 0000000000..2ce7156677
--- /dev/null
+++ b/src/Illuminate/Queue/UniqueHandler.php
@@ -0,0 +1,114 @@
+<?php
+
+namespace Illuminate\Queue;
+
+use Illuminate\Bus\UniqueLock;
+use Illuminate\Contracts\Cache\Factory as CacheFactory;
+use Illuminate\Contracts\Container\Container;
+use Illuminate\Contracts\Queue\ShouldBeUnique;
+use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
+
+/**
+ * A helper class to manage the unique ID and cache instance for a job
+ * base on the data of the job itself.
+ */
+class UniqueHandler
+{
+ /**
+ * Original job name
+ *
+ * @var string
+ */
+ public $jobName;
+
+ /**
+ * The unique ID for the job.
+ *
+ * @var string|null
+ */
+ public $uniqueId = null;
+
+ /**
+ * Cache connection name for the job.
+ *
+ * @var string|null
+ */
+ protected $uniqueVia = null;
+
+ /**
+ * The container instance.
+ * @var \Illuminate\Contracts\Container\Container
+ */
+ protected $container;
+
+ /**
+ * Create a new handler instance.
+ *
+ * @param object $job
+ */
+ public function __construct(object $job)
+ {
+ $this->jobName = get_class($job);
+
+ if (method_exists($job, 'uniqueId')) {
+ $this->uniqueId = $job->uniqueId();
+ } else if (isset($job->uniqueId)) {
+ $this->uniqueId = $job->uniqueId;
+ }
+
+ if (method_exists($job, 'uniqueVia')) {
+ $this->uniqueVia = $job->uniqueVia()->getName();
+ }
+ }
+
+ /**
+ * Creates a new instance if the job should be unique.
+ *
+ * @param object $job
+ * @return \Illuminate\Queue\UniqueHandler|null
+ */
+ public static function forJob(object $job)
+ {
+ if (
+ $job instanceof ShouldBeUnique ||
+ $job instanceof ShouldBeUniqueUntilProcessing
+ ) {
+ return new static($job);
+ }
+
+ return null;
+ }
+
+ /**
+ * Sets the container instance.
+ *
+ * @param \Illuminate\Contracts\Container\Container $container
+ * @return \Illuminate\Queue\UpdateHandler
+ */
+ public function withContainer(Container $container)
+ {
+ $this->container = $container;
+ return $this;
+ }
+
+ /**
+ * Returns the cache instance for the job.
+ *
+ * @return \Illuminate\Contracts\Cache\Repository
+ */
+ protected function getCacheStore()
+ {
+ return $this->container->make(CacheFactory::class)
+ ->store($this->uniqueVia);
+ }
+
+ /**
+ * Releases the lock for the job.
+ *
+ * @return void
+ */
+ public function release()
+ {
+ (new UniqueLock($this->getCacheStore()))->release($this);
+ }
+} EDIT: doc comments types updated. |
Hi @naquad, can you attempt that PR? Thanks! |
@driesvints I've sent a PR with the fix #50211. |
I'm sorry @naquad but it seems that this is a no-fix for us right now. If you could produce a much simpler PR like Taylor requested, we'd love to review it. Thanks |
Laravel Version
10.42.0
PHP Version
8.3.2
Database Driver & Version
Redis 7.2.4
Description
Queued jobs with ShouldBeUnique may cause stale unique locks in the case when the dependent model is missing.
According to the source code of the CallQueuedHandler::call() unique lock cleanup may never be reached in case if the job depends on the missing model AND the job is configured to be deleted when the model is missing (
public $deleteWhenMissingModels = true
).Steps To Reproduce
The PoC is made using Redis queue, a similar approach may work with other drivers.
laravel new poc
..env
file. WARNING: PoC will flush all keys.php poc.php
Output
Expected result
The unique lock has been removed.
Actual result
A stale unique lock still exists.
The text was updated successfully, but these errors were encountered: