framework

framework copied to clipboard

Laravel Version

10.48.12 (although I believe relevant code has not changed in 11.x)

PHP Version

8.1.28

Database Driver & Version

Redis 7.2.5 (although issue should exist with other cache drivers)

Description

When dispatching jobs, the ShouldBeUnique behavior is only respected when dispatching through the PendingDispatch class and the Illuminate\Console\Scheduling\Schedule class (the latter fix was added in #39302). However, it is not respected when using the Illuminate\Bus\Dispatcher class or individual queue drivers (Queue::push( ... ), etc). This behavior can be confusing because depending on how you queue the job, it's easily possible to queue duplicates, despite the job implementing ShouldBeUnique

I found a previous PR (#50381) that attempted to move the ShouldBeUnique check into the Dispatcher class. Although that still wouldn't guarantee uniqueness when using Queue::push(). I was also able to find some other issues which seem to indicate that people are generally expecting ShouldBeUnique to work universally:

• #39113
• #45781
• #42297
• #48882

There's also a mention in the original PR for the ShouldBeUnique feature mentioning how this could possibly be moved into the enqueueUsing() logic in the Queue class? This seems like it would probably fix all of the above issues

Further, the Dispatcher class and Queue enqueueUsing() method already check other "expressive" interfaces like ShouldQueue, ShouldQueueAfterCommit, etc, so it seems reasonable to also check ShouldBeUnique there

I would be willing to open a PR for this if desired. Was hesitant to do so as a similar PR (#50381) has already been closed

Steps To Reproduce

Create a job which implements ShouldBeUnique

<?php

namespace App\Jobs;

use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;

class UniqueJob implements ShouldQueue, ShouldBeUnique
{
    use Dispatchable;

    ...
}

Confirm that ShouldBeUnique works correctly when using PendingDispatch. In tinker

Queue::size(); // Returns 0
UniqueJob::dispatch();
Queue::size(); // Returns 1. May have to call this multiple times, presumably to trigger the `__destruct()` method in `PendingDispatch`, but it will queue the job eventually
UniqueJob::dispatch();
Queue::size(); // Still returns 1, as `ShouldBeUnique` prevents queuing

Confirm that using Queue::push(new UniqueJob()), app(Dispatcher::class)->dispatch(new UniqueJob()), Bus::dispatch(new UniqueJob()) do not work the same way

Queue::size(); // Returns 0
Queue::push(new UniqueJob());
Queue::size(); // Returns 1
Queue::push(new UniqueJob());
Queue::size(); // Returns 2. One would expect a "unique" job to not be queued again

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!

As Taylor said we're tabling this one for now.

@driesvints I'm nobody to question the maintenance team. But I did want to point out:

(While I'm not intimately familiar with the internals)

Taylor's comments makes it seem as though only undocumented dispatch methods are affected.

But that's not the case.

Using ShouldBeUnique as documented, with the documented Bus::chain, and Bus::batch methods does not guarantee uniqueness.

The only way to guarantee a unique job is going through PendingDispatch, which Bus::chain and Bus::batch do not.

I'm pretty confident, This is a broken feature. (10x, 11x, and 12x)

Got burned by this today... Our team was trying to standardize on using facades across our code base, but with this it means we can't use Bus::dispatch() as you'd expect.

There's another comment here from October on my original PR that attempted to fix this. So seems like people are still finding this to be an issue, or at the very least finding the behavior to be unexpected.

If a fix for this isn't desired, I do think a note could be added to the docs at the very least. Perhaps one of these blurbs

That says something like @shawnnaquin said above "The only way to guarantee a unique job is going through PendingDispatch, ..." or something similar

So seems like people are still finding this to be an issue, or at the very least finding the behavior to be unexpected.

I can sign this one.

We implemented some jobs using ShouldBeUnique some years ago and, I could swear, it worked. Some days ago I realized it didn't and the reason was because due to DI we were using \Illuminate\Contracts\Bus\Dispatcher directly and therefore it didn't work. Using the global function dispatch() worked out for us.

Seeing how only that code makes use of PendingDispatch which takes care of ShouldBeUnique I guess it makes sense, but it's not clear from any docs or anything.

To be fair, the docs only use ::dispatch() as example (at least on the Queue docs page, where the unique feature is explained), which comes from the Dispatchable trait, which itself also uses PendingDispatch.

Does anyone have a dirty fix for Bus::dispatch and guarantee the job uniqueness? I was ready to do bad things to my computer before I found about this PR, at least update documentation so that other don't fall into the same mistake.

@vladyslavkotyk If you must use Bus::dispatch, I'd think your best bet is to copy the shouldDispatch() logic out of PendingDispatch, then override the standard Dispatcher implementation including the shouldDispatch() logic and bind your new implementation. Or just manually include that shouldDispatch() logic in your own code, even

If you don't need Bus::dispatch, it's probably easier to just use another queueing method that does support uniqueness, like the dispatch() helper or Job::dispatch()