laravel-bridge icon indicating copy to clipboard operation
laravel-bridge copied to clipboard

Queue worker does not respect `maxExceptions`

Open marcusrettig opened this issue 2 years ago • 8 comments

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.

marcusrettig avatar Jun 12 '23 14:06 marcusrettig

@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?

mnapoli avatar Jun 12 '23 15:06 mnapoli

Jup, the queue worker depends on cache to work.

georgeboot avatar Jun 12 '23 15:06 georgeboot

@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?

mnapoli avatar Jun 12 '23 21:06 mnapoli

@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.

marcusrettig avatar Jun 14 '23 08:06 marcusrettig

@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.

mnapoli avatar Jun 15 '23 16:06 mnapoli

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

marcusrettig avatar Jun 16 '23 08:06 marcusrettig

@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.

marcusrettig avatar Jun 19 '23 12:06 marcusrettig

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

mnapoli avatar Jun 21 '23 09:06 mnapoli