Skip to content
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

[Cache] Potential Premature Release of Unique Lock in Queues #52980

Closed
felipehertzer opened this issue Sep 30, 2024 · 1 comment
Closed

[Cache] Potential Premature Release of Unique Lock in Queues #52980

felipehertzer opened this issue Sep 30, 2024 · 1 comment

Comments

@felipehertzer
Copy link

Laravel Version

11

PHP Version

8.3

Database Driver & Version

No response

Description

Hi,

I'm testing jobs using ShouldBeUnique with the $uniqueFor parameter. From my understanding, $uniqueFor should prevent any other job with the same unique ID from being dispatched within the specified timeframe. However, I noticed that the lock is released immediately after the job is processed, rather than respecting the timeout period. This allows the same job to be dispatched again before the intended expiration time.

Expected Behaviour:
The lock should remain for the full $uniqueFor duration, even after the job is processed.

Actual Behaviour:
The lock is removed as soon as the job is processed, ignoring the $uniqueFor setting.

Is this the intended behaviour, or should the expiration time be enforced?

Thanks.

Steps To Reproduce

Environment Variables:

CACHE_DRIVER=database
QUEUE_CONNECTION=database

Job Dispatch:

MyJob::dispatch(id: 1);

Job Class:

<?php

namespace App\Jobs;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;

class MyJob implements ShouldBeUnique, ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public int $uniqueFor = 3600; // Unique for 1 hour

    /**
     * Create a new job instance.
     */
    public function __construct(
        protected int $id,
    ) {}

    /**
     * Execute the job.
     */
    public function handle(): void
    {
        // Job logic here...
    }

    /**
     * The unique ID of the job.
     */
    public function uniqueId(): string
    {
        return "$this->id";
    }
}

Observed Behaviour

After dispatching the job, the cache_locks table contains the following row:

key                                                | owner               | expiration
'laravel_unique_job:App\\Jobs\\MyJob:1'             | 'zSjQSs37Fn7gKEYA'  | 1727654333

However, when the queue worker is started using:

./vendor/bin/sail artisan queue:work

As soon the job is processed the lock is removed from the cache looks.

Investigation

It appears that the lock is being released in CallQueuedHandler.php:

if (! $job->isReleased() && ! $command instanceof ShouldBeUniqueUntilProcessing) {
    $this->ensureUniqueJobLockIsReleased($command);
}

This triggers a call to UniqueLock.php which uses forceRelease:

$cache->lock($this->getKey($job))->forceRelease();

Question

Shouldn't the expiration time be checked before releasing the lock? Is it expected that the lock is released regardless of the job's uniqueFor timeout, or am I missing some configuration or implementation detail that would prevent this behaviour?

Thank you!

Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants