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] Check attempts before firing queue job. #15319

Merged
merged 4 commits into from
Sep 7, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Illuminate/Queue/AttemptsExceededException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

namespace Illuminate\Queue;

use RuntimeException;

class AttemptsExceededException extends RuntimeException
{
//
}
45 changes: 45 additions & 0 deletions src/Illuminate/Queue/Worker.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -250,6 +253,31 @@ 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.
*
Expand All @@ -266,6 +294,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.
Expand Down
30 changes: 27 additions & 3 deletions tests/Queue/QueueWorkerTest.php
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;
Expand Down Expand Up @@ -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) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this extra line

Copy link
Author

Choose a reason for hiding this comment

The 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]));
Expand All @@ -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...
*/
Expand Down Expand Up @@ -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()
Expand Down