From de2e30a80943702166f0e2cce9cf088a64a9e435 Mon Sep 17 00:00:00 2001 From: Max Brokman Date: Wed, 7 Sep 2016 12:20:10 +0100 Subject: [PATCH 1/4] Check jobs before working to see if they have already been received too 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. --- .../Queue/AttemptsExceededException.php | 12 ++++++ src/Illuminate/Queue/Worker.php | 43 +++++++++++++++++++ tests/Queue/QueueWorkerTest.php | 30 +++++++++++-- 3 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 src/Illuminate/Queue/AttemptsExceededException.php diff --git a/src/Illuminate/Queue/AttemptsExceededException.php b/src/Illuminate/Queue/AttemptsExceededException.php new file mode 100644 index 000000000000..3500c8fc7328 --- /dev/null +++ b/src/Illuminate/Queue/AttemptsExceededException.php @@ -0,0 +1,12 @@ +raiseBeforeJobEvent($connectionName, $job); + // Check if this job has already been received too many times + $this->markJobAsFailedIfAlreadyExceedsMaxAttempts($connectionName, $job, $options->maxTries); + // Here we will fire off the job and let it process. We will catch any exceptions so // they can be reported to the developers logs, etc. Once the job is finished the // proper events will be fired to let any listeners know this job has finished. @@ -250,6 +253,29 @@ protected function handleJobException($connectionName, $job, WorkerOptions $opti throw $e; } + /** + * Mark the given job as failed if it has exceeded the maximum allowed attempts. This will likely be because + * the job previously exceeded a timeout. + * + * @param string $connectionName + * @param \Illuminate\Contracts\Queue\Job $job + * @param int $maxTries + * @return void + */ + protected function markJobAsFailedIfAlreadyExceedsMaxAttempts($connectionName, $job, $maxTries) + { + if ($maxTries === 0 || $job->attempts() <= $maxTries) { + return; + } + + $e = new AttemptsExceededException( + "Queue job has already been attempted more than maxTries, it may have previously timed out"); + + $this->failJob($connectionName, $job, $e); + + throw $e; + } + /** * Mark the given job as failed if it has exceeded the maximum allowed attempts. * @@ -266,6 +292,23 @@ protected function markJobAsFailedIfHasExceededMaxAttempts( return; } + $this->failJob($connectionName, $job, $e); + } + + /** + * Mark the given job as failed and raise the relevant event. + * + * @param string $connectionName + * @param \Illuminate\Contracts\Queue\Job $job + * @param \Exception $e + * @return void + */ + protected function failJob($connectionName, $job, $e) + { + if ($job->isDeleted()) { + return; + } + // If the job has failed, we will delete it, call the "failed" method and then call // an event indicating the job has failed so it can be logged if needed. This is // to allow every developer to better keep monitor of their failed queue jobs. diff --git a/tests/Queue/QueueWorkerTest.php b/tests/Queue/QueueWorkerTest.php index 2ace0a9adad7..46b0b9f7a2b5 100755 --- a/tests/Queue/QueueWorkerTest.php +++ b/tests/Queue/QueueWorkerTest.php @@ -1,5 +1,6 @@ attempts++; + throw $e; }); - $job->attempts = 5; + $job->attempts = 1; $worker = $this->getWorker('default', ['queue' => [$job]]); $worker->runNextJob('default', 'queue', $this->workerOptions(['maxTries' => 1])); @@ -106,6 +111,25 @@ public function test_job_is_not_released_if_it_has_exceeded_max_attempts() $this->events->shouldNotHaveReceived('fire', [Mockery::type(JobProcessed::class)]); } + public function test_job_is_failed_if_it_has_already_exceeded_max_attempts() + { + $job = new WorkerFakeJob(function ($job) { + $job->attempts++; + }); + $job->attempts = 2; + + $worker = $this->getWorker('default', ['queue' => [$job]]); + $worker->runNextJob('default', 'queue', $this->workerOptions(['maxTries' => 1])); + + $this->assertNull($job->releaseAfter); + $this->assertTrue($job->deleted); + $this->assertInstanceOf(AttemptsExceededException::class, $job->failedWith); + $this->exceptionHandler->shouldHaveReceived('report')->with(Mockery::type(AttemptsExceededException::class)); + $this->events->shouldHaveReceived('fire')->with(Mockery::type(JobExceptionOccurred::class))->once(); + $this->events->shouldHaveReceived('fire')->with(Mockery::type(JobFailed::class))->once(); + $this->events->shouldNotHaveReceived('fire', [Mockery::type(JobProcessed::class)]); + } + /** * Helpers... */ @@ -212,7 +236,7 @@ public function __construct($callback = null) public function fire() { $this->fired = true; - $this->callback->__invoke(); + $this->callback->__invoke($this); } public function payload() From 7520913992c177b51689e8073d81a2353402f5e0 Mon Sep 17 00:00:00 2001 From: Max Brokman Date: Wed, 7 Sep 2016 12:22:36 +0100 Subject: [PATCH 2/4] Style fixes for de2e30a80943702166f0e2cce9cf088a64a9e435 --- src/Illuminate/Queue/AttemptsExceededException.php | 2 -- src/Illuminate/Queue/Worker.php | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Illuminate/Queue/AttemptsExceededException.php b/src/Illuminate/Queue/AttemptsExceededException.php index 3500c8fc7328..6e1cd15894f9 100644 --- a/src/Illuminate/Queue/AttemptsExceededException.php +++ b/src/Illuminate/Queue/AttemptsExceededException.php @@ -1,9 +1,7 @@ failJob($connectionName, $job, $e); From 73752e0a2332a6927bedd6872be04077d7faaa3d Mon Sep 17 00:00:00 2001 From: Max Brokman Date: Wed, 7 Sep 2016 12:27:24 +0100 Subject: [PATCH 3/4] Style fixes suggested in laravel/framework#15319 --- src/Illuminate/Queue/Worker.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Queue/Worker.php b/src/Illuminate/Queue/Worker.php index 45f5497dcf25..e9ddeebfcb00 100644 --- a/src/Illuminate/Queue/Worker.php +++ b/src/Illuminate/Queue/Worker.php @@ -254,8 +254,9 @@ protected function handleJobException($connectionName, $job, WorkerOptions $opti } /** - * Mark the given job as failed if it has exceeded the maximum allowed attempts. This will likely be because - * the job previously exceeded a timeout. + * Mark the given job as failed if it has exceeded the maximum allowed attempts. + * + * This will likely be because the job previously exceeded a timeout. * * @param string $connectionName * @param \Illuminate\Contracts\Queue\Job $job @@ -269,7 +270,8 @@ protected function markJobAsFailedIfAlreadyExceedsMaxAttempts($connectionName, $ } $e = new AttemptsExceededException( - 'Queue job has already been attempted more than maxTries, it may have previously timed out'); + 'Queue job has already been attempted more than maxTries, it may have previously timed out' + ); $this->failJob($connectionName, $job, $e); From 54d2c6b8e38e90da7466fb5f306e1dbbe08a28e9 Mon Sep 17 00:00:00 2001 From: Max Brokman Date: Wed, 7 Sep 2016 13:42:01 +0100 Subject: [PATCH 4/4] Remove extra line --- tests/Queue/QueueWorkerTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Queue/QueueWorkerTest.php b/tests/Queue/QueueWorkerTest.php index 46b0b9f7a2b5..eaa84dfb0237 100755 --- a/tests/Queue/QueueWorkerTest.php +++ b/tests/Queue/QueueWorkerTest.php @@ -91,7 +91,6 @@ public function test_job_is_not_released_if_it_has_exceeded_max_attempts() $e = new RuntimeException; $job = new WorkerFakeJob(function ($job) use ($e) { - // In normal use this would be incremented by being popped off the queue $job->attempts++;