brefphp / laravel-bridge

Package to use Laravel on AWS Lambda with Bref
https://bref.sh/docs/frameworks/laravel.html
MIT License
314 stars 63 forks source link

Queue worker does not respect `maxExceptions` #121

Open marcusrettig opened 1 year ago

marcusrettig commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Jup, the queue worker depends on cache to work.

mnapoli commented 1 year ago

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

Do we have 2 problems here:

or do we have just one?

marcusrettig commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

marcusrettig commented 1 year ago

@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 commented 1 year ago

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