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

[6.x] Reset timeout handler after worker loop #31198

Merged
merged 2 commits into from Jan 22, 2020
Merged

Conversation

@themsaid
Copy link
Member

themsaid commented Jan 22, 2020

If we have a timeout of 5 seconds, and a job took 4 seconds to run. On the next loop if getNextJob() took 1 second or more to run, the worker will exit before the script reaches registerTimeoutHandler() due to timeout. This will cause the job to not run, yet the attempt will count.

In this PR, we reset the alarm so it doesn't kill the process before the new loop sets a new value for the alarm.

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 22, 2020

This also introduces the possibility of a totally frozen worker if the code gets stuck on retrieving the next job and the client does not have a proper timeout in place. I'm not sure if any queue drivers are vulnerable to this.

@themsaid

This comment has been minimized.

Copy link
Member Author

themsaid commented Jan 22, 2020

@taylorotwell I think for at least the drivers recommended for production (redis and sqs), an exception gets thrown if the connection fails. I understand your concern but I think this issue is the cause of the mysterious "Job has been attempted too many times or run too long" error while nothing was attempted.

@taylorotwell taylorotwell merged commit e18a984 into laravel:6.x Jan 22, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.