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.7] Fix an issue where the worker process would not be killed by the listener when the timeout is exceeded #25981

Merged
merged 4 commits into from
Oct 23, 2018

Conversation

jshayes
Copy link

@jshayes jshayes commented Oct 7, 2018

Refer to #25980 for the issue.

This PR fixes the issue by passing an array into the Process constructor rather than the formatted stripe. Refer to symfony/symfony#21474 for the reason for passing an array in.

Here are some test cases that I've tried to show how they differ. I ran php artisan queue:listen database --timeout 10 on the 5.7 branch and on my branch. Then I ran ps -ef | grep "[q]ueue". The results from that second command are as follows:

On the 5.7 branch, on Mac:

$ ps -ef | grep "[q]ueue"
  501 13085 12444   0  1:16am ttys007    0:00.11 php artisan queue:listen database --timeout 10
  501 13089 13085   0  1:16am ttys007    0:00.12 /usr/local/Cellar/php/7.2.1_12/bin/php artisan queue:work database --once --queue=default --delay=0 --memory=128 --sleep=3 --tries=0

On the 5.7 branch, on Linux:

$ ps -ef | grep "[q]ueue"
vagrant   5512  5485 46 05:21 pts/1    00:00:00 php artisan queue:listen database --timeout 10
vagrant   5521  5512  0 05:21 pts/1    00:00:00 sh -c '/usr/bin/php7.2' 'artisan' queue:work 'database' --once --queue='default' --delay=0 --memory=128 --sleep=3 --tries=0
vagrant   5522  5521  0 05:21 pts/1    00:00:00 /usr/bin/php7.2 artisan queue:work database --once --queue=default --delay=0 --memory=128 --sleep=3 --tries=0

On the my branch, on Mac:

$ ps -ef | grep "[q]ueue"
  501 13818  9815   0  1:22am ttys001    0:00.14 php artisan queue:listen database --timeout 10
  501 13846 13818   0  1:22am ttys001    0:00.13 /usr/local/Cellar/php/7.2.1_12/bin/php artisan queue:work database --once --queue=default --delay=0 --memory=128 --sleep=3 --tries=0

On the my branch, on Linux:

$ ps -ef | grep "[q]ueue"
vagrant   5536  5485 46 05:21 pts/1    00:00:00 php artisan queue:listen database --timeout 10
vagrant   5540  5536  0 05:21 pts/1    00:00:00 /usr/bin/php7.2 artisan queue:work database --once --queue=default --delay=0 --memory=128 --sleep=3 --tries=0

As you can see, on my branch the listener correctly forks the worker command, which will allow the SIGTERM signal to terminate the correct process.

Let me know what you think about this approach, and if you know any issues with this.

@TBlindaruk TBlindaruk changed the title Fix an issue where the worker process would not be killed by the listener when the timeout is exceeded [5.7] Fix an issue where the worker process would not be killed by the listener when the timeout is exceeded Oct 7, 2018
@taylorotwell
Copy link
Member

When testing this in my own applications:

Symfony\Component\Debug\Exception\FatalThrowableError : Argument 1 passed to Symfony\Component\Process\Process::escapeArgument() must be of the type string, null given

@jshayes
Copy link
Author

jshayes commented Oct 17, 2018

Good catch, I always specified a connection when calling the listener.

@taylorotwell taylorotwell merged commit 30316d9 into laravel:5.7 Oct 23, 2018
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.

2 participants