From f160ce5959054c17adb3df42167069700b1401e4 Mon Sep 17 00:00:00 2001 From: Marc Hagen Date: Thu, 29 Aug 2024 13:15:24 +0200 Subject: [PATCH 1/5] Add test to verify STDOUT unserialization is successful --- phpunit.xml.dist | 1 + tests/ErrorHandlingTest.php | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 531071a4..d2c5a444 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -12,6 +12,7 @@ convertWarningsToExceptions="true" processIsolation="false" stopOnFailure="false" + beStrictAboutTestsThatDoNotTestAnything="true" > diff --git a/tests/ErrorHandlingTest.php b/tests/ErrorHandlingTest.php index 468b0794..1156ebd9 100644 --- a/tests/ErrorHandlingTest.php +++ b/tests/ErrorHandlingTest.php @@ -167,6 +167,22 @@ public function it_handles_stderr_as_parallel_error() $pool->wait(); } + /** @test */ + public function it_handles_stdout_as_parallel_error() + { + $pool = Pool::create(); + + $pool->add(function () { + fwrite(STDOUT, 'test'); + })->then(function ($output) { + $this->fail('Child process output did not error on faulty output'); + })->catch(function (ParallelError $error) { + $this->assertStringContainsString('test', $error->getMessage()); + }); + + $pool->wait(); + } + /** @test */ public function deep_syntax_errors_are_thrown() { From b9dec0864b2c94cf15ef581b8972a9126da1e232 Mon Sep 17 00:00:00 2001 From: Marc <980978+MarcHagen@users.noreply.github.com> Date: Wed, 28 Aug 2024 23:14:06 +0200 Subject: [PATCH 2/5] Fail if `unserialize` is unable to parse output In `ParallelProcess` in `getOutput()` is checked if the `unserialize` is successful. When it fails, is sets the `errorOutput` to the process output. But this is not validated anymore because the check is already passed. Moving the `getOutput()` before the error output check will ensure all error output cases are handled properly. --- src/Process/ProcessCallbacks.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Process/ProcessCallbacks.php b/src/Process/ProcessCallbacks.php index 16a70a21..52f99451 100644 --- a/src/Process/ProcessCallbacks.php +++ b/src/Process/ProcessCallbacks.php @@ -34,14 +34,14 @@ public function timeout(callable $callback): self public function triggerSuccess() { + $output = $this->getOutput(); + if ($this->getErrorOutput()) { $this->triggerError(); return; } - $output = $this->getOutput(); - foreach ($this->successCallbacks as $callback) { call_user_func_array($callback, [$output]); } From 7a0b08cb3abdcef2bfb41906da7afaf615c8925b Mon Sep 17 00:00:00 2001 From: Marc <980978+MarcHagen@users.noreply.github.com> Date: Wed, 28 Aug 2024 23:26:51 +0200 Subject: [PATCH 3/5] Make own output from the child process so we can check on it --- src/Process/ParallelProcess.php | 16 +++++++++++----- src/Runtime/ChildRuntime.php | 4 ++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/Process/ParallelProcess.php b/src/Process/ParallelProcess.php index 9e95cb8f..b36040a6 100644 --- a/src/Process/ParallelProcess.php +++ b/src/Process/ParallelProcess.php @@ -68,11 +68,15 @@ public function getOutput() if (! $this->output) { $processOutput = $this->process->getOutput(); - $this->output = @unserialize(base64_decode($processOutput)); + $childResult = @unserialize(base64_decode($processOutput)); - if (! $this->output) { + if ($childResult === false || ! array_key_exists('output', $childResult)) { $this->errorOutput = $processOutput; + + return null; } + + $this->output = $childResult['output']; } return $this->output; @@ -83,11 +87,13 @@ public function getErrorOutput() if (! $this->errorOutput) { $processOutput = $this->process->getErrorOutput(); - $this->errorOutput = @unserialize(base64_decode($processOutput)); + $childResult = @unserialize(base64_decode($processOutput)); - if (! $this->errorOutput) { + if ($childResult === false || ! array_key_exists('output', $childResult)) { $this->errorOutput = $processOutput; - } + } else { + $this->errorOutput = $childResult['output']; + } } return $this->errorOutput; diff --git a/src/Runtime/ChildRuntime.php b/src/Runtime/ChildRuntime.php index b1e805bd..5224fa75 100644 --- a/src/Runtime/ChildRuntime.php +++ b/src/Runtime/ChildRuntime.php @@ -25,7 +25,7 @@ $output = call_user_func($task); - $serializedOutput = base64_encode(serialize($output)); + $serializedOutput = base64_encode(serialize(['output' => $output])); if (strlen($serializedOutput) > $outputLength) { throw \Spatie\Async\Output\ParallelError::outputTooLarge($outputLength); @@ -39,7 +39,7 @@ $output = new \Spatie\Async\Output\SerializableException($exception); - fwrite(STDERR, base64_encode(serialize($output))); + fwrite(STDERR, base64_encode(serialize(['output' => $output]))); exit(1); } From ac545b6b23a2a8ebb933a086bbe25e9ff4967f50 Mon Sep 17 00:00:00 2001 From: Marc Hagen Date: Thu, 29 Aug 2024 00:50:13 +0200 Subject: [PATCH 4/5] Catch `php://output` and add to output. Any writes to `php://stdout` will break the unserialization of child process results --- src/Runtime/ChildRuntime.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/Runtime/ChildRuntime.php b/src/Runtime/ChildRuntime.php index 5224fa75..47f4c462 100644 --- a/src/Runtime/ChildRuntime.php +++ b/src/Runtime/ChildRuntime.php @@ -2,6 +2,16 @@ use Spatie\Async\Runtime\ParentRuntime; +// php://stdout does not obey output buffering. Any output would break +// unserialization of child process results in the parent process. +if (!defined('STDOUT')) { + define('STDOUT', fopen('php://temp', 'w+b')); + define('STDERR', fopen('php://stderr', 'wb')); +} + +ini_set('display_startup_errors', 1); +ini_set('display_errors', 'stderr'); + try { $autoloader = $argv[1] ?? null; $serializedClosure = $argv[2] ?? null; @@ -23,7 +33,9 @@ $task = ParentRuntime::decodeTask($serializedClosure); + ob_start(); $output = call_user_func($task); + ob_end_clean(); $serializedOutput = base64_encode(serialize(['output' => $output])); From bc87ce661a2290e6f65ececab2d5dc8ebe5b940a Mon Sep 17 00:00:00 2001 From: Marc Hagen Date: Thu, 29 Aug 2024 13:53:51 +0200 Subject: [PATCH 5/5] Fix child test (should have never worked) But it did because the child process output was "childTjs=" but this is incorrect and cannot be unserialized. --- tests/ChildRuntimeTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/ChildRuntimeTest.php b/tests/ChildRuntimeTest.php index 3a7a2405..f63fe707 100644 --- a/tests/ChildRuntimeTest.php +++ b/tests/ChildRuntimeTest.php @@ -15,7 +15,8 @@ public function it_can_run() $autoloader = __DIR__.'/../vendor/autoload.php'; $serializedClosure = base64_encode(serialize(new SerializableClosure(function () { - echo 'child'; + echo 'interfere with output'; + return 'child'; }))); $process = new Process([ @@ -28,7 +29,10 @@ public function it_can_run() $process->start(); $process->wait(); + $output = unserialize(base64_decode($process->getOutput())); - $this->assertStringContainsString('child', $process->getOutput()); + $this->assertIsArray($output); + $this->assertArrayHasKey('output', $output); + $this->assertStringContainsString('child', $output['output']); } }