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] Catch errors when handling a failed job #16212

Merged
merged 14 commits into from
Nov 2, 2016
Merged

[5.3] Catch errors when handling a failed job #16212

merged 14 commits into from
Nov 2, 2016

Conversation

brianmclachlin
Copy link

More explanation in my comment on this issue

TLDR;
If there is any problems with the payload of a job then the failed job event will not fire and the job is lost.

Brian McLachlin added 2 commits November 1, 2016 11:18
Handles errors when calling the failed method on a job. Could be
ReflectionException or an exception with unserializing the payload.
@taylorotwell
Copy link
Member

Thanks for tracking down this issue. I wonder if there is a more specific way to catch this error. It seems like could also be catching unrelated errors in this try / catch block and immediately marking things as failed for other reasons? Maybe that's what we want to happen? Do you have any thoughts there?

Brian McLachlin added 6 commits November 1, 2016 16:35
failJob gets called from 2 separate places. Better to put this code
around a try catch instead of in the try catch in handleJobException
… fix-queue-failed-job-event

# Conflicts:
#	src/Illuminate/Queue/Worker.php
@brianmclachlin
Copy link
Author

brianmclachlin commented Nov 1, 2016

@taylorotwell I believe that we would want to catch all errors. We need the failed job event to get fired; otherwise, jobs will be lost. The smallest change I can think of is using a try catch to ensure that the failed job event gets fired. I can't think of any other solutions without doing larger changes to the Worker class.

After looking at this issue some more I realized that failJob also gets called from markJobAsFailedIfAlreadyExceedsMaxAttempts before the job is fired. This would also cause the job to skip the failed_jobs table.

Instead of putting a catch in handleJobException I thought it'd be better to wrap the logic in failJob with a try catch of it's own. This way the use case is handled from both spots where failJob is called. It will catch the exception and always run $this->raiseFailedJobEvent($connectionName, $job, $e); no matter what.

@taylorotwell
Copy link
Member

And this still fixes the issue on your end?

@brianmclachlin
Copy link
Author

Yup this still fixes the issue on our end, I ran through the test case again to be sure.

@jesseleite
Copy link

jesseleite commented Nov 1, 2016

It seems like could also be catching unrelated errors in this try / catch block and immediately marking things as failed for other reasons? Maybe that's what we want to happen? Do you have any thoughts there?

@taylorotwell I believe that we would want to catch all errors. We need the failed job event to get fired; otherwise, jobs will be lost.

100% agree with @brianmclachlin. It doesn't matter where the exception comes from, I'm under the opinion that the failing job should be sent to failed jobs table with the recorded exception in all cases. A failed job should only be removed from the failed jobs table if: A) A developer runs artisan queue:retry and the job is successful, or B) A developer runs artisan queue:flush. The recorded payload and exception are valuable for debugging purposes. My 2 cents anyway.

@brianmclachlin
Copy link
Author

@taylorotwell Not sure why travis is failing. The job log doesn't give me any output.

@brianmclachlin
Copy link
Author

@taylorotwell Travis for some reason had output this morning. Figured out what broke the tests and fixed it.

@taylorotwell taylorotwell merged commit f052310 into laravel:5.3 Nov 2, 2016
@jesseleite
Copy link

Thank you @taylorotwell 👌

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

Successfully merging this pull request may close these issues.

3 participants