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

[5.3] Daemon queue worker: Jobs that exceed timeout will never 'fail' #15317

Closed
maxbrokman opened this issue Sep 7, 2016 · 1 comment
Closed

Comments

@maxbrokman
Copy link

  • Laravel Version: 5.3.6
  • PHP Version: 5.6.19|7.0.4
  • Database Driver & Version: Mysql (MariaDB 10)
  • Queue Driver: Redis, but generally applicable.

Description:

The timeout option when running artisan queue:work --daemon --timeout=n --tries=n successfully kills child processes that run too long but they will never go through the job failure process.

Jobs are marked as failed if they throw an exception during execution and have already been received by a worker too many times. This logic is handled in this method, which is only called from Worker::process() (through handleJobException).

This works well for normal jobs but jobs that exceed the timeout will never actually reach this point, as the parent process kills them while they are still working the logic to 'fail' a job will run.

Steps To Reproduce:

Send jobs to the queue that exceed the timeout and then attempt to work them, for example

public function handle()
{
    echo "Job has been received" . $this->attempts() . " times" . PHP_EOL;
    sleep(10);
}

Working the queue with php artisan queue:work --daemon --timeout=5 --tries=1 I would expect to see this job appear once in output and then fail (ending up the in failed_jobs table etc).

Instead the output is along the lines of

Job has been received 1 times
Job has been received 2 times
Job has been received 3 times
Job has been received 4 times
...

Suggested fix

Move the check for a job that has exceeded its maximum attempts to before the job fires. A job that has repeatedly timed out will then fail the next time the worker receives it.

I'm going to submit a PR for this shortly.

maxbrokman pushed a commit to maxbrokman/framework that referenced this issue Sep 7, 2016
…oo many times.

Resolves an issue with the `--timeout` feature where jobs the repeatedly timed out would never be marked as `failed`, as the worker process would be killed before
it could reach the failing logic.

To maintain compatibility there are now two checks against the number of attempts a job has had, one before working the job and one in the case of an job raising an exception.

see laravel#15317 for more details.
symfony-splitter pushed a commit to illuminate/queue that referenced this issue Sep 7, 2016
…oo many times.

Resolves an issue with the `--timeout` feature where jobs the repeatedly timed out would never be marked as `failed`, as the worker process would be killed before
it could reach the failing logic.

To maintain compatibility there are now two checks against the number of attempts a job has had, one before working the job and one in the case of an job raising an exception.

see laravel/framework#15317 for more details.
@maxbrokman
Copy link
Author

Fixed in #15319 (hopefully :p)

taylorotwell pushed a commit to illuminate/queue that referenced this issue Sep 17, 2018
…oo many times.

Resolves an issue with the `--timeout` feature where jobs the repeatedly timed out would never be marked as `failed`, as the worker process would be killed before
it could reach the failing logic.

To maintain compatibility there are now two checks against the number of attempts a job has had, one before working the job and one in the case of an job raising an exception.

see laravel/framework#15317 for more details.
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

1 participant