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

Queue worker does not respect maxExceptions #121

Open
marcusrettig opened this issue Jun 12, 2023 · 8 comments
Open

Queue worker does not respect maxExceptions #121

marcusrettig opened this issue Jun 12, 2023 · 8 comments

Comments

@marcusrettig
Copy link
Contributor

Setting the maxExceptions property on a job has no effect. For instance, the following job will be tried 25 times even though it throws an exception every time:

class ProcessPodcast implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public $tries = 25;
    public $maxExceptions = 3;

    public function handle(): void {
        throw new Exception('The podcast cannot be processed');
    }
}

I believe this is because the base worker class uses the cache to count the number of exceptions (see the markJobAsFailedIfWillExceedMaxExceptions method). The cache is set with $worker->setCache(..) in Laravels WorkCommand, but I don't believe this is done in Laravel Bridge.

@mnapoli
Copy link
Member

mnapoli commented Jun 12, 2023

@tillkruss you're using this extensively I believe, any idea?

Also even if the cache was set, I believe the user must be using DynamoDB or Redis as a centralized cache (else this is the disk cache), so that might be worth clarifying in the docs, right?

@georgeboot
Copy link
Contributor

Jup, the queue worker depends on cache to work.

@mnapoli
Copy link
Member

mnapoli commented Jun 12, 2023

@georgeboot but as @marcusrettig pointed out, it seems the cache is not set?

Do we have 2 problems here:

  • the cache instance is not set on the worker
  • the documentation doesn't indicate that one must use a centralized cache

or do we have just one?

@marcusrettig
Copy link
Contributor Author

@mnapoli Thank you for your quick response! I do believe we have both problems. A centralized cache would only be required for jobs that use maxExceptions though.

@mnapoli
Copy link
Member

mnapoli commented Jun 15, 2023

@marcusrettig got it! Would you be able to test in your case that with ->setCache (and after setting a centralized cache) things work correctly?

Then if you have time for a PR we can merge it. If you don't I could try to work on it but it would take more time because I'm tight on time right now.

@marcusrettig
Copy link
Contributor Author

marcusrettig commented Jun 16, 2023

@mnapoli I will test it later today or early next week, and if it solves the issue I can open a PR.

@marcusrettig
Copy link
Contributor Author

@mnapoli I created #122 with the basic fix. I tested it on a single worker (maxConcurrency: 1) using file cache, and it worked. I assume it will also work using a centralized cache, but I will have to verify this later in our staging environment.

@mnapoli
Copy link
Member

mnapoli commented Jun 21, 2023

Thanks @marcusrettig!

I'm keeping this issue open because we need to document that either Redis or DynamoDB must be used as Laravel cache on this page: https://bref.sh/docs/frameworks/laravel.html#laravel-queues

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

No branches or pull requests

3 participants