From 07c92648845ce1f207008e2d74e2d4d03b57b766 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Kapp=C3=A9?= <tobias.kappe@gmail.com>
Date: Sat, 27 Aug 2016 12:56:13 +0200
Subject: [PATCH] Read standard output and standard error of child process
 using stream_select.

If a process filled up the buffer of the standard error pipe before closing the
standard output pipe, it would hang. As a result, the call to
stream_get_contents for the standard output pipe would not return, as the
standard error pipe needs to be read from in order for the process to continue.
This change uses stream_select to read from both pipes whenever data becomes
available.
---
 PHPCI/Helper/BaseCommandExecutor.php       | 45 +++++++++++++++++++---
 Tests/PHPCI/Helper/CommandExecutorTest.php | 21 ++++++++++
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/PHPCI/Helper/BaseCommandExecutor.php b/PHPCI/Helper/BaseCommandExecutor.php
index b3b47f7b..8831eaaa 100644
--- a/PHPCI/Helper/BaseCommandExecutor.php
+++ b/PHPCI/Helper/BaseCommandExecutor.php
@@ -92,14 +92,14 @@ abstract class BaseCommandExecutor implements CommandExecutor
         $pipes = array();
         $process = proc_open($command, $descriptorSpec, $pipes, $this->buildPath, null);
 
+        $this->lastOutput = '';
+        $this->lastError = '';
+
         if (is_resource($process)) {
             fclose($pipes[0]);
 
-            $this->lastOutput = stream_get_contents($pipes[1]);
-            $this->lastError = stream_get_contents($pipes[2]);
-
-            fclose($pipes[1]);
-            fclose($pipes[2]);
+            list($this->lastOutput, $this->lastError) =
+                $this->readAlternating([$pipes[1], $pipes[2]]);
 
             $status = proc_close($process);
         }
@@ -125,6 +125,41 @@ abstract class BaseCommandExecutor implements CommandExecutor
         return $rtn;
     }
 
+    /**
+     * Reads from array of streams as data becomes available.
+     * @param array     $descriptors
+     * @return string[] data read from each descriptor
+     */
+    private function readAlternating(array $descriptors)
+    {
+        $outputs = [];
+        foreach ($descriptors as $key => $descriptor) {
+            stream_set_blocking($descriptor, false);
+            $outputs[$key] = '';
+        }
+
+        do {
+            $read = $descriptors;
+            $write = null;
+            $except = null;
+
+            stream_select($read, $write, $except, null);
+
+            foreach ($read as $descriptor) {
+                $key = array_search($descriptor, $descriptors);
+
+                if (feof($descriptor)) {
+                    fclose($descriptor);
+                    unset($descriptors[$key]);
+                } else {
+                    $outputs[$key] .= fgets($descriptor);
+                }
+            }
+        } while (count($descriptors) > 0);
+
+        return $outputs;
+    }
+
     /**
      * Returns the output from the last command run.
      */
diff --git a/Tests/PHPCI/Helper/CommandExecutorTest.php b/Tests/PHPCI/Helper/CommandExecutorTest.php
index 5d5dc08b..2a31b8c9 100644
--- a/Tests/PHPCI/Helper/CommandExecutorTest.php
+++ b/Tests/PHPCI/Helper/CommandExecutorTest.php
@@ -58,6 +58,27 @@ class CommandExecutorTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($returnValue);
     }
 
+    /**
+     * Runs a script that generates an output that fills the standard error
+     * buffer first, followed by the standard output buffer. The function
+     * should be able to read from both streams, thereby preventing the child
+     * process from blocking because one of its buffers is full.
+     */
+    public function testExecuteCommand_AlternatesBothBuffers()
+    {
+        $length = 80000;
+        $script = <<<EOD
+/bin/sh -c 'data="$(printf %%${length}s | tr " " "-")"; >&2 echo "\$data"; >&1 echo "\$data"'
+EOD;
+        $data = str_repeat("-", $length);
+
+        $returnValue = $this->testedExecutor->executeCommand(array($script));
+        $this->assertTrue($returnValue);
+
+        $this->assertEquals($data, trim($this->testedExecutor->getLastOutput()));
+        $this->assertEquals($data, trim($this->testedExecutor->getLastError()));
+    }
+
     public function testFindBinary_ReturnsPathInSpecifiedRoot()
     {
         $thisFileName = "CommandExecutorTest.php";