fix!: handle errors properly

src/Child.php

@@ -4,6 +4,8 @@ declare(strict_types=1);
 
 namespace Nih\CommandBuilder;
 
+use InvalidArgumentException;
+
 /**
  * Representation of a running or exited child process.
  */
@@ -26,6 +28,11 @@ final class Child
      */
     private ?ExitStatus $status = null;
 
+    /**
+     * The child's output.
+     */
+    private ?Output $output = null;
+
     /**
      * The handle for writing to the child’s standard input (stdin), if it has
      * been captured.
@@ -45,13 +52,17 @@ final class Child
     public readonly ?ChildStderr $stderr;
 
     /**
-     * @param resource $process The child's process handle.
-     * @param array<int, resource> $pipes File pointers.
+     * @param resource $process The child's process resource handle.
+     * @param array<int, resource> $pipes
+     * The indexed array of file pointers that set by {@see \proc_open()}.
+     *
+     * @throws InvalidArgumentException
+     * When the given process is not a process resource.
      */
     public function __construct(mixed $process, array $pipes)
     {
         if (get_resource_type($process) !== 'process') {
-            throw new ChildException('invalid resource type: not a process');
+            throw new InvalidArgumentException('resource is not a process');
         }
 
         $this->process = $process;
@@ -89,8 +100,6 @@ final class Child
      * waiting. This helps avoid deadlock: it ensures that the child does not
      * block waiting for input from the parent, while the parent waits for the
      * child to exit.
-     *
-     * @throws ChildException If the resource was already closed
      */
     public function wait(): ExitStatus
     {
@@ -111,7 +120,7 @@ final class Child
         proc_close($this->process);
 
         return $this->status = new ExitStatus(
-            $status['exitcode'],
+            $status['exitcode'] < 0 ? null : $status['exitcode'],
             $status['signaled'] ? $status['termsig'] : null,
             $status['stopped'] ? $status['stopsig'] : null,
         );
@@ -120,6 +129,8 @@ final class Child
     /**
      * Simultaneously waits for the child to exit and collect all remaining
      * output on the stdout/stderr handles, returning an {@see Output} instance.
+     * This function will continue to have the same return value after it has
+     * been called at least once.
      *
      * The stdin handle to the child process, if any, will be closed before
      * waiting. This helps avoid deadlock: it ensures that the child does not
@@ -131,12 +142,13 @@ final class Child
      * parent and child. Use the `stdout` and `stderr` functions of {@see
      * Command}, respectively.
      *
-     * @throws ChildException If the resource was already closed
+     * @throws ChildException
+     * @throws StreamException
      */
     public function waitWithOutput(): Output
     {
-        if (!is_resource($this->process)) {
-            throw new ChildException('Resource was already closed');
+        if ($this->output) {
+            return $this->output;
         }
 
         // Avoid possible deadlock before waiting.
@@ -146,21 +158,29 @@ final class Child
         $stderr = $this->stderr?->getContents();
         $status = $this->wait();
 
-        return new Output($stdout, $stderr, $status);
+        return $this->output = new Output($stdout, $stderr, $status);
     }
 
     /**
      * Forces the child process to exit.
      *
      * This is equivalent to sending a SIGKILL.
+     *
+     * @throws ChildException
      */
-    public function kill(): bool
+    public function kill(): void
     {
-        if (!is_resource($this->process)) {
-            return true;
+        if ($this->status) {
+            return;
         }
 
-        return proc_terminate($this->process, 9);
+        try {
+            set_error_handler(ChildException::handleError(...));
+            $success = proc_terminate($this->process, 9);
+            assert($success);
+        } finally {
+            restore_error_handler();
+        }
     }
 
     public function __destruct()

src/ChildException.php

@@ -4,5 +4,23 @@ declare(strict_types=1);
 
 namespace Nih\CommandBuilder;
 
-class ChildException extends CommandException
-{}
+/**
+ * A PHP error occured during an operation on a process resource.
+ */
+final class ChildException extends CommandException
+{
+    public static function handleError(
+        int $severity,
+        string $message,
+        string $filename,
+        int $line,
+    ): never {
+        throw new self(
+            message: $message,
+            code: 0,
+            severity: $severity,
+            filename: $filename,
+            line: $line,
+        );
+    }
+}

src/Command.php

@@ -278,6 +278,8 @@ final class Command implements Stringable
      * Executes the command as a child process, returning a handle to it.
      *
      * By default, stdin, stdout and stderr are inherited from the parent.
+     *
+     * @throws ChildException
      */
     public function spawn(): Child
     {
@@ -299,6 +301,8 @@ final class Command implements Stringable
      * collecting its status.
      *
      * By default, stdin, stdout and stderr are inherited from the parent.
+     *
+     * @throws ChildException
      */
     public function status(): ExitStatus
     {
@@ -311,6 +315,8 @@ final class Command implements Stringable
      *
      * By default, stdout and stderr are captured (and used to provide the
      * resulting output). Stdin is not inherited from the parent.
+     *
+     * @throws ChildException
      */
     public function output(): Output
     {
@@ -396,13 +402,6 @@ final class Command implements Stringable
             }
         }
 
-        if (!is_executable($program)) {
-            throw new CommandException(sprintf(
-                'Program "%s" is not executable',
-                $program,
-            ));
-        }
-
         // Handle environment inheritance.
         $environment = $this->environment;
         if (is_array($environment) && $this->environmentInherit) {
@@ -414,7 +413,7 @@ final class Command implements Stringable
         }
 
         try {
-            set_error_handler(CommandException::handleError(...));
+            set_error_handler(ChildException::handleError(...));
             $proc = proc_open(
                 [$program, ...$this->args],
                 $descriptorSpec,

src/CommandException.php

@@ -4,16 +4,9 @@ declare(strict_types=1);
 
 namespace Nih\CommandBuilder;
 
-use RuntimeException;
+use ErrorException;
 
-class CommandException extends RuntimeException
-{
-    public static function handleError(int $errno, string $errstr, string $file, int $line): never
-    {
-        $exception = new self($errstr);
-        $exception->file = $file;
-        $exception->line = $line;
-
-        throw $exception;
-    }
-}
+/**
+ *
+ */
+abstract class CommandException extends ErrorException {}

src/ExitStatus.php

@@ -27,7 +27,9 @@ final readonly class ExitStatus
      */
     public function success(): bool
     {
-        return $this->code === 0;
+        return $this->code === 0
+            && $this->signal === null
+            && $this->stoppedSignal === null;
     }
 
     /**

src/StreamException.php

@@ -5,6 +5,38 @@ declare(strict_types=1);
 namespace Nih\CommandBuilder;
 
 /**
- * An error prevented reading from or writing to a stream resource.
+ * A PHP error occured during an operation on a stream resource.
  */
-class StreamException extends CommandException {}
+final class StreamException extends CommandException
+{
+    /**
+     * A note on handling IO errors by setting an error handler before each
+     * operation: Yes, this is slower, but not by much (1.5x).
+     *
+     * However, it is even slower if wrapped with another function, so it is better
+     * to do this inline every time before the function (e.g. fopen) is called.
+     *
+     * ```php
+     * try {
+     *     set_error_handler(CommandException::handleError(...));
+     *     $handle = fopen('foo', 'r');
+     * } finally {
+     *     restore_error_handler();
+     * }
+     * ```
+     */
+    public static function handleError(
+        int $severity,
+        string $message,
+        string $filename,
+        int $line,
+    ): never {
+        throw new self(
+            message: $message,
+            code: 0,
+            severity: $severity,
+            filename: $filename,
+            line: $line,
+        );
+    }
+}