From 1e15e88acc536f1e9fdeabe92ca3d4793855e0a4 Mon Sep 17 00:00:00 2001 From: Justin Hayes Date: Sun, 7 Oct 2018 01:08:35 -0400 Subject: [PATCH 1/4] Fix an issue where the worker process would not be killed by the listener when the timeout is exceeded Refer to #25980 --- src/Illuminate/Queue/Listener.php | 73 +++++++++++-------------------- tests/Queue/QueueListenerTest.php | 18 +++++++- 2 files changed, 42 insertions(+), 49 deletions(-) diff --git a/src/Illuminate/Queue/Listener.php b/src/Illuminate/Queue/Listener.php index f4862ec8d30f..813842f1475f 100755 --- a/src/Illuminate/Queue/Listener.php +++ b/src/Illuminate/Queue/Listener.php @@ -3,7 +3,6 @@ namespace Illuminate\Queue; use Closure; -use Illuminate\Support\ProcessUtils; use Symfony\Component\Process\Process; use Symfony\Component\Process\PhpExecutableFinder; @@ -37,13 +36,6 @@ class Listener */ protected $maxTries = 0; - /** - * The queue worker command line. - * - * @var string - */ - protected $workerCommand; - /** * The output handler callback. * @@ -60,19 +52,6 @@ class Listener public function __construct($commandPath) { $this->commandPath = $commandPath; - $this->workerCommand = $this->buildCommandTemplate(); - } - - /** - * Build the environment specific worker command. - * - * @return string - */ - protected function buildCommandTemplate() - { - $command = 'queue:work %s --once --queue=%s --delay=%s --memory=%s --sleep=%s --tries=%s'; - - return "{$this->phpBinary()} {$this->artisanBinary()} {$command}"; } /** @@ -82,9 +61,7 @@ protected function buildCommandTemplate() */ protected function phpBinary() { - return ProcessUtils::escapeArgument( - (new PhpExecutableFinder)->find(false) - ); + return (new PhpExecutableFinder)->find(false); } /** @@ -94,9 +71,7 @@ protected function phpBinary() */ protected function artisanBinary() { - return defined('ARTISAN_BINARY') - ? ProcessUtils::escapeArgument(ARTISAN_BINARY) - : ProcessUtils::escapeArgument('artisan'); + return defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan'; } /** @@ -126,22 +101,20 @@ public function listen($connection, $queue, ListenerOptions $options) */ public function makeProcess($connection, $queue, ListenerOptions $options) { - $command = $this->workerCommand; + // First, we will just create the worker commands with all of the various + // options available for the command. This will produce the final command + // line array that we will pass into a Symfony process object for processing. + $command = $this->createCommand( + $connection, $queue, $options + ); - // If the environment is set, we will append it to the command string so the + // If the environment is set, we will append it to the command array so the // workers will run under the specified environment. Otherwise, they will // just run under the production environment which is not always right. if (isset($options->environment)) { $command = $this->addEnvironment($command, $options); } - // Next, we will just format out the worker commands with all of the various - // options available for the command. This will produce the final command - // line that we will pass into a Symfony process object for processing. - $command = $this->formatCommand( - $command, $connection, $queue, $options - ); - return new Process( $command, $this->commandPath, null, null, $options->timeout ); @@ -152,31 +125,35 @@ public function makeProcess($connection, $queue, ListenerOptions $options) * * @param string $command * @param \Illuminate\Queue\ListenerOptions $options - * @return string + * @return array */ protected function addEnvironment($command, ListenerOptions $options) { - return $command.' --env='.ProcessUtils::escapeArgument($options->environment); + return array_merge($command, ["--env={$options->environment}"]); } /** - * Format the given command with the listener options. + * Create the command with the listener options. * - * @param string $command * @param string $connection * @param string $queue * @param \Illuminate\Queue\ListenerOptions $options * @return string */ - protected function formatCommand($command, $connection, $queue, ListenerOptions $options) + protected function createCommand($connection, $queue, ListenerOptions $options) { - return sprintf( - $command, - ProcessUtils::escapeArgument($connection), - ProcessUtils::escapeArgument($queue), - $options->delay, $options->memory, - $options->sleep, $options->maxTries - ); + return [ + $this->phpBinary(), + $this->artisanBinary(), + 'queue:work', + $connection, + '--once', + "--queue={$queue}", + "--delay={$options->delay}", + "--memory={$options->memory}", + "--sleep={$options->sleep}", + "--tries={$options->maxTries}", + ]; } /** diff --git a/tests/Queue/QueueListenerTest.php b/tests/Queue/QueueListenerTest.php index 868201043a52..fd1e565b9542 100755 --- a/tests/Queue/QueueListenerTest.php +++ b/tests/Queue/QueueListenerTest.php @@ -49,6 +49,22 @@ public function testMakeProcessCorrectlyFormatsCommandLine() $this->assertInstanceOf(Process::class, $process); $this->assertEquals(__DIR__, $process->getWorkingDirectory()); $this->assertEquals(3, $process->getTimeout()); - $this->assertEquals($escape.PHP_BINARY.$escape." {$escape}artisan{$escape} queue:work {$escape}connection{$escape} --once --queue={$escape}queue{$escape} --delay=1 --memory=2 --sleep=3 --tries=0", $process->getCommandLine()); + $this->assertEquals($escape.PHP_BINARY.$escape." {$escape}artisan{$escape} {$escape}queue:work{$escape} {$escape}connection{$escape} {$escape}--once{$escape} {$escape}--queue=queue{$escape} {$escape}--delay=1{$escape} {$escape}--memory=2{$escape} {$escape}--sleep=3{$escape} {$escape}--tries=0{$escape}", $process->getCommandLine()); + } + + public function testMakeProcessCorrectlyFormatsCommandLineWithAnEnvironmentSpecified() + { + $listener = new Listener(__DIR__); + $options = new ListenerOptions('test'); + $options->delay = 1; + $options->memory = 2; + $options->timeout = 3; + $process = $listener->makeProcess('connection', 'queue', $options); + $escape = '\\' === DIRECTORY_SEPARATOR ? '"' : '\''; + + $this->assertInstanceOf(Process::class, $process); + $this->assertEquals(__DIR__, $process->getWorkingDirectory()); + $this->assertEquals(3, $process->getTimeout()); + $this->assertEquals($escape.PHP_BINARY.$escape." {$escape}artisan{$escape} {$escape}queue:work{$escape} {$escape}connection{$escape} {$escape}--once{$escape} {$escape}--queue=queue{$escape} {$escape}--delay=1{$escape} {$escape}--memory=2{$escape} {$escape}--sleep=3{$escape} {$escape}--tries=0{$escape} {$escape}--env=test{$escape}", $process->getCommandLine()); } } From 03944206259e3803eaaee0e5c16a3ab7d3686818 Mon Sep 17 00:00:00 2001 From: Justin Hayes Date: Wed, 17 Oct 2018 10:08:22 -0400 Subject: [PATCH 2/4] Added failing test --- tests/Queue/QueueListenerTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/Queue/QueueListenerTest.php b/tests/Queue/QueueListenerTest.php index fd1e565b9542..e1ee9694bdf2 100755 --- a/tests/Queue/QueueListenerTest.php +++ b/tests/Queue/QueueListenerTest.php @@ -67,4 +67,20 @@ public function testMakeProcessCorrectlyFormatsCommandLineWithAnEnvironmentSpeci $this->assertEquals(3, $process->getTimeout()); $this->assertEquals($escape.PHP_BINARY.$escape." {$escape}artisan{$escape} {$escape}queue:work{$escape} {$escape}connection{$escape} {$escape}--once{$escape} {$escape}--queue=queue{$escape} {$escape}--delay=1{$escape} {$escape}--memory=2{$escape} {$escape}--sleep=3{$escape} {$escape}--tries=0{$escape} {$escape}--env=test{$escape}", $process->getCommandLine()); } + + public function testMakeProcessCorrectlyFormatsCommandLineWhenTheConnectionIsNotSpecified() + { + $listener = new Listener(__DIR__); + $options = new ListenerOptions('test'); + $options->delay = 1; + $options->memory = 2; + $options->timeout = 3; + $process = $listener->makeProcess(null, 'queue', $options); + $escape = '\\' === DIRECTORY_SEPARATOR ? '"' : '\''; + + $this->assertInstanceOf(Process::class, $process); + $this->assertEquals(__DIR__, $process->getWorkingDirectory()); + $this->assertEquals(3, $process->getTimeout()); + $this->assertEquals($escape.PHP_BINARY.$escape." {$escape}artisan{$escape} {$escape}queue:work{$escape} {$escape}--once{$escape} {$escape}--queue=queue{$escape} {$escape}--delay=1{$escape} {$escape}--memory=2{$escape} {$escape}--sleep=3{$escape} {$escape}--tries=0{$escape} {$escape}--env=test{$escape}", $process->getCommandLine()); + } } From 275e0f05ce474d1372e21a536841e9c73114929f Mon Sep 17 00:00:00 2001 From: Justin Hayes Date: Wed, 17 Oct 2018 10:12:13 -0400 Subject: [PATCH 3/4] Filter out the null connection --- src/Illuminate/Queue/Listener.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Illuminate/Queue/Listener.php b/src/Illuminate/Queue/Listener.php index 813842f1475f..160cbb834119 100755 --- a/src/Illuminate/Queue/Listener.php +++ b/src/Illuminate/Queue/Listener.php @@ -105,7 +105,9 @@ public function makeProcess($connection, $queue, ListenerOptions $options) // options available for the command. This will produce the final command // line array that we will pass into a Symfony process object for processing. $command = $this->createCommand( - $connection, $queue, $options + $connection, + $queue, + $options ); // If the environment is set, we will append it to the command array so the @@ -116,7 +118,11 @@ public function makeProcess($connection, $queue, ListenerOptions $options) } return new Process( - $command, $this->commandPath, null, null, $options->timeout + $command, + $this->commandPath, + null, + null, + $options->timeout ); } @@ -142,7 +148,7 @@ protected function addEnvironment($command, ListenerOptions $options) */ protected function createCommand($connection, $queue, ListenerOptions $options) { - return [ + return array_filter([ $this->phpBinary(), $this->artisanBinary(), 'queue:work', @@ -153,7 +159,7 @@ protected function createCommand($connection, $queue, ListenerOptions $options) "--memory={$options->memory}", "--sleep={$options->sleep}", "--tries={$options->maxTries}", - ]; + ]); } /** From 30316d98a2efb5abfecc2a98120192e11fa424fa Mon Sep 17 00:00:00 2001 From: Justin Hayes Date: Wed, 17 Oct 2018 10:30:38 -0400 Subject: [PATCH 4/4] Only filter out null values --- src/Illuminate/Queue/Listener.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Queue/Listener.php b/src/Illuminate/Queue/Listener.php index 160cbb834119..e3f2d9049f8f 100755 --- a/src/Illuminate/Queue/Listener.php +++ b/src/Illuminate/Queue/Listener.php @@ -159,7 +159,9 @@ protected function createCommand($connection, $queue, ListenerOptions $options) "--memory={$options->memory}", "--sleep={$options->sleep}", "--tries={$options->maxTries}", - ]); + ], function ($value) { + return ! is_null($value); + }); } /**