[8.x] Fix scheduler unique job

[ericsssan]

Hi maintainers

I discovered a bug where scheduling job that implemented ShouldBeUnique failed to enforce uniqueness.

I have included test case and possible fix.

Please review.

Thanks

[taylorotwell]

@paras-malhotra do you have any thoughts on this?

[paras-malhotra]

I dont believe unique jobs were ever meant for task scheduling. @ericsssan what is your use case here and can it not be solved using withoutOverlapping or onOneServer?

[ericsssan]

Hi

Use case:
I have a job called ProcessDelayedSMSToSendOut. This job runs every minute to query db to get a list of sms notifications and put them into Redis queue to be sent out. This job sometimes take more than a minute to complete. If it takes more than a minute the scheduler will dispatch another job overlapping the previous job which results in duplicate sms being sent out unintentionally. This happened to us. Some recipients received up to 5 identical sms at once and it went on for months.

Objective:
What we don't want to happen is to have multiple instance of the above mentioned job in the queue at any one time. We have limited resource on the redis server. Using ShouldBeUnqiue interface works for us. The problem is with the job method.

What works:

• I can dispatch the job from a command class.
• I can dispatch the job from schedule->call() callback.

What does not work:

• Dispatch with schedule->job()

https://laravel.com/docs/8.x/scheduling#scheduling-queued-jobs

According to the doc , job method in Task Scheduling is a convenient method for call. But dispatching a unique job from scheduler within call method callback works as expected whereas dispatching with job method does not.

If you believe that sheduler is not meant to dispatch unique job then I think we can just put it in the doc to warn others about it.

I just think that dispatching a job should work as expected regardless of where you dispatch it. But the problem is we get a different dispatcher when we dispatch with schedule->job() and that makes it inconsistent.

Cheers
Eric

[taylorotwell]

Do you have a good understanding of why your change fixes it? If so, can you explain?

[ericsssan]

Hi @taylorotwell

Yes.

When dispatching a job elsewhere it goes through ShouldDispatch method in PendingDispatch class.

$uniqueJob::dispatch(); or dispatch(new UniqueJob);

But when using $schedule->job(), it does not go through PendingDispatch class.

framework/src/Illuminate/Console/Scheduling/Schedule.php

Line 142 in 41ac7e8

$this->dispatchToQueue($job, $queue ?? $job->queue, $connection ?? $job->connection);

Here is the job method call dispatchToQueue method.

framework/src/Illuminate/Console/Scheduling/Schedule.php

Line 171 in 41ac7e8

$this->getDispatcher()->dispatch(

Then goes on to call getDispatcher method and chain call dispatch method.

framework/src/Illuminate/Console/Scheduling/Schedule.php

Line 311 in 41ac7e8

protected function getDispatcher()

And here, getDispatcher simply just resolve Dispatcher from a container and dispatch without going through PendingDispatch class.

I can think of 2 ways to fix this:

1. in dispatchToQueue method, just use dispatch helper function to dispatch the job. But using helper function is probably a bad idea.

- $this->getDispatcher()->dispatch(
-           $job->onConnection($connection)->onQueue($queue)
-        );
+ dispatch($job)->onConnection($connection)->onQueue($queue);

2. do exactly what the dispatch helper function does and be more explicit.

Please let me know if I missed out something.

Cheers
Eric

[taylorotwell]

Replacing with #39302 because using Foundation classes from the console component isn't really allowed. Had to refactor some things into the Bus component.