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

Conversation

maxbrokman
Copy link

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 #15317 for more details.

Max Brokman added 2 commits September 7, 2016 12:20
…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.
@GrahamCampbell GrahamCampbell changed the title Check attempts before firing queue job. [5.3] Check attempts before firing queue job Sep 7, 2016
@maxbrokman maxbrokman changed the title [5.3] Check attempts before firing queue job [5.3] Check attempts before firing queue job. Sep 7, 2016
@@ -251,6 +254,29 @@ 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
Copy link
Member

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. :)

Copy link
Author

@maxbrokman maxbrokman Sep 7, 2016

Choose a reason for hiding this comment

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

Like this?

    /**
     * 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
     */

Copy link
Member

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.

@GrahamCampbell
Copy link
Member

Thanks for the PR. :)

@maxbrokman
Copy link
Author

Pushed up your suggested style fixes GC. Let me know on any further

@@ -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

@bronxct1
Copy link

bronxct1 commented Sep 11, 2016

I've also noticed the above issue @landish reported, you cannot just run php artisan queue:listen without setting --tries to at least 1

This could be resolved by changing the conditional on line 269 of src/Illuminate/Queue/Worker.php

from

if ($maxTries === 0 || $job->attempts() <= $maxTries) {

to

if ($maxTries == 0 || $job->attempts() <= $maxTries) {

Basically, $maxTries comes in as a string so it will always result in false.

I'm going to PR this fix.

Also @maxbrokman nice work on this PR. Hope all is well.

@GrahamCampbell
Copy link
Member

Looks like a bug. The default was meant to be unlimited retrys.

@drakakisgeo
Copy link

I have the same problem as @landish says about the "markJobAsFailedIfAlreadyExceedsMaxAttempts" exception.

@vlakoff
Copy link
Contributor

vlakoff commented Sep 17, 2016

"maxTries 0" bug fixed in 5.3.9, refs #15370 and #15390.

@zirikatzaile
Copy link

Hi ! My case it is , I think, subtly different. I recently moved from 5.2 to 5.3 and my scheduled commands including these lines

app/Console/Kernel.php:33:      $schedule->command('queue:work')->everyFiveMinutes();
app/Console/Kernel.php:34:      $schedule->command('queue:work --queue=import')->everyFiveMinutes();

are causing hundred of MySQL never dying processes (quite long Sleep status and mounting up).

I've searched a lot and the closest issue is this one, but it does not look to be exact the same as, among others, I do not see any exceeded time exception in logs.

taylorotwell pushed a commit to illuminate/queue that referenced this pull request Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants