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

[8.x] Add job middleware to prevent overlapping jobs #34794

Merged
merged 9 commits into from Oct 14, 2020
Merged

[8.x] Add job middleware to prevent overlapping jobs #34794

merged 9 commits into from Oct 14, 2020

Conversation

paras-malhotra
Copy link
Contributor

@paras-malhotra paras-malhotra commented Oct 11, 2020

Laravel ships many routing middleware classes out of the box, but there are no job middleware classes yet. I think it may be very useful if we also introduce job middleware out of the box for common use cases.

One common use case is to avoid overlapping jobs. An example use case could be to avoid processing a double refund on an order.

This PR introduces a PreventOverlappingJobs middleware that does just that (prevents overlapping jobs). For the refund processing job example above, we could use this middleware like so:

public function middleware()
{
    return [
        new PreventOverlappingJobs($this->order->id)
    ];
}

If there is a need to avoid overlapping jobs globally (instead of say by order), we could use it as mentioned below. An example use case could be say updating the search index when a searchable model is updated.

public function middleware()
{
    return [new PreventOverlappingJobs];
}

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Oct 12, 2020

@taylorotwell thanks for your review. I've fixed exception handling and also added a test to confirm that:

  1. Locks are released on job exceptions
  2. Exceptions are passed through so that the queue worker can handle them

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Oct 12, 2020

Also added an option to expire the lock to handle queue worker crashes (if needed).

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 12, 2020

So if the lock already exists... the job is just deleted? Am I missing something? That doesn't seem like desirable behavior.

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Oct 12, 2020

If the lock exists, the job is "skipped" as it is considered to a duplicate concurrent job. This is similar to the behaviour of a scheduled cron job when run with withoutOverlapping. Some use cases would be initiating a refund on the same order twice while the refund is processing, request to update the search index while the indexing is taking place, etc.

I do understand that there can be more use cases where users might want the job released back to the queue. So, I think I'll add an option to do so to cover more use cases.

@paras-malhotra paras-malhotra marked this pull request as draft Oct 12, 2020
@paras-malhotra paras-malhotra marked this pull request as ready for review Oct 12, 2020
@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Oct 12, 2020

@taylorotwell I've updated the middleware so that the default behaviour is to release the job back to the queue with an option to not release (or delete on overlap).

This code releases back to the queue on overlap:

public function middleware()
{
    return [
        new PreventOverlappingJobs($this->order->id)
    ];
}

This code doesn't release (deletes) on overlap:

public function middleware()
{
    return [
        (new PreventOverlappingJobs($this->order->id))->dontRelease()
    ];
}

This code releases after a 60 seconds delay:

public function middleware()
{
    return [
        (new PreventOverlappingJobs($this->order->id))->releaseAfter(60)
    ];
}

You could also automatically expire the lock to take care of queue worker crashes:

public function middleware()
{
    return [
        (new PreventOverlappingJobs($this->order->id))->releaseAfter(60)->expireAt(3600)
    ];
}

@themsaid
Copy link
Member

@themsaid themsaid commented Oct 13, 2020

I think this middleware can be more useful if you can configure the maximum concurrency.

Something like:

public function middleware()
{
    return [
        new LimitConcurrency($this->user->id, 5)
    ];
}

This will prevent a certain user from having this job running more than 5 instances at the same time.

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 13, 2020

@themsaid that could probably be a separate middleware.

@paras-malhotra
Copy link
Contributor Author

@paras-malhotra paras-malhotra commented Oct 13, 2020

Thanks @themsaid and @taylorotwell, let me know if you'd like me to make any changes.

@taylorotwell taylorotwell merged commit 2a587e5 into laravel:8.x Oct 14, 2020
7 checks passed
@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 14, 2020

Renamed to WithoutOverlapping.

@liamkeily
Copy link

@liamkeily liamkeily commented Jan 6, 2022

When stress testing this middleware i easily hit the attempts limit because each time the job is released back to the queue it counts as another attempt. I can get around this by defining a retryUntil on the job, but i'm wondering if it is expected behaviour to increment attempts in this case?

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

Successfully merging this pull request may close these issues.

None yet

5 participants