-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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] Check attempts before firing queue job. #15319
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<?php | ||
|
||
namespace Illuminate\Queue; | ||
|
||
use RuntimeException; | ||
|
||
class AttemptsExceededException extends RuntimeException | ||
{ | ||
// | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,9 @@ public function process($connectionName, $job, WorkerOptions $options) | |
try { | ||
$this->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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please put the closing bracket on the next line. |
||
|
||
$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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
<?php | ||
|
||
use Illuminate\Queue\AttemptsExceededException; | ||
use Illuminate\Queue\WorkerOptions; | ||
use Illuminate\Queue\Events\JobFailed; | ||
use Illuminate\Queue\Events\JobProcessed; | ||
|
@@ -89,10 +90,14 @@ public function test_job_is_not_released_if_it_has_exceeded_max_attempts() | |
{ | ||
$e = new RuntimeException; | ||
|
||
$job = new WorkerFakeJob(function () use ($e) { | ||
$job = new WorkerFakeJob(function ($job) use ($e) { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove this extra line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 54d2c6b |
||
// In normal use this would be incremented by being popped off the queue | ||
$job->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() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the rest of the description two lines down. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly. I really meant two newline characters. :)
Just one star inbetween please.