laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

queue-worker + supervisor: if a job calls `exit()` it will be attempted an infinite number of times #1147

Open milsanore opened 6 years ago

milsanore commented 6 years ago

Hi,

I'm not sure if this is a bug or a feature, so I thought I'd place it under ideas to start with.

First my env:

My issue is this: I have built my entire back-end using Laravel microservices / php7.1 (shout out to Mark Zuckerberg), and it is working well.

I use Laravel as a distributed queue-worker (dozens of workers), and I manage the queue-worker process using supervisor. Under some exceptional conditions I do not want supervisor to restart the queue-worker process (i.e. something has gone horribly wrong). I can achieve this using exit(99); combined with a supervisor conf entry exitcodes=99.

However, because I'm calling exit(99) from my job class' handle() function, Laravel does not get the opportunity to re-enqueue the job with an incremented attempts count, which means the job will be attempted an infinite number of times (and would ultimately bring down the entire fleet, but that's incidental).

Ideally, I would like to throw an Exception, which would allow Laravel to cleanup/re-enqueue the job, before exiting with an exit-code, but I suspect Laravel does not currently support this. Maybe Laravel could monitor if an exception has an exception code, and if so exit() after cleanup.

Thanks,

sisve commented 6 years ago

However, because I'm calling exit(99) from my job class' handle() function, Laravel does not get the opportunity to re-enqueue the job with an incremented attempts count, which means the job will be attempted an infinite number of times [...]

The attempts counter should be increased when the job is fetched from the source, not when the job fails. This is easiest shown in the DatabaseQueue::pop method where we fetch the next available job, and mark it as reserved (in the marshalJob method which calls markJobAsReserved).

Anyhow, it's up to the different queue implementations to do this, or similar logic. I use the database queue as a reference point because it's easy to understand since we're in control of the entire flow. In your case it sounds like your queue driver (fhteam/laravel-amqp) isn't handling the scenario correctly. I would try and file an issue with them at https://github.com/fhteam/laravel-amqp/issues since it falls outside the scope of laravel/framework.

Ideally, I would like to throw an Exception, which would allow Laravel to cleanup/re-enqueue the job, before exiting with an exit-code, but I suspect Laravel does not currently support this.

Use the InteractsWithQueue trait and call $this->delete() to remove the job from the queue before calling exit().

milsanore commented 6 years ago

Looking through the source of fhteam/laravel-amqp, the job is incremented in the Job->release() function, which seems reasonable. I don't think the question of where the job counter is incremented is the problem; the problem is that I need to exit(99), which means that I would have to explicitly release the job back onto the queue myself, before I fire exit(99). In its current implementation, Laravel can handle job exceptions, and trigger the release of the job back onto the queue, but exit() interrupts this flow. It would be great if there was an Exception type that would allow Laravel to exit() at the end of its job-exception-handling code

sisve commented 6 years ago

All of the framework queue drivers support what you want because they increase the attempts before executing your job. That's how they know if a job has fatally crashed enough times to be marked as failed.

The problem is entirely in the choice to increase the attempts after the job. There's also an reported issue at https://github.com/fhteam/laravel-amqp/issues/21 already. While that issue is about fatal errors, you're asking about the same thing, a sudden abrupt end of execution.