diff --git a/composer-require-check.json b/composer-require-check.json index 6c1ee5d..50e8694 100644 --- a/composer-require-check.json +++ b/composer-require-check.json @@ -20,7 +20,11 @@ "escapeArgument", "IS_WINDOWS", "posix_kill", + "posix_get_last_error", + "posix_strerror", "pcntl_waitpid", + "pcntl_get_last_error", + "PCNTL_EINTR", "WNOHANG" ], "php-core-extensions": [ diff --git a/src/Internal/Posix/PosixHandle.php b/src/Internal/Posix/PosixHandle.php index 9dc774e..60afc33 100644 --- a/src/Internal/Posix/PosixHandle.php +++ b/src/Internal/Posix/PosixHandle.php @@ -37,7 +37,7 @@ public function __construct( $stdin = \WeakReference::create($stdin); $this->extraDataPipeCallbackId = EventLoop::unreference(EventLoop::onReadable( $extraDataPipe, - static function (string $callbackId, $stream) use (&$status, $deferred, $stdin, $shellPid): void { + static function (string $callbackId, $stream) use (&$status, $deferred, $stdin, $proc, $shellPid): void { EventLoop::disable($callbackId); $status = ProcessStatus::Ended; @@ -56,7 +56,7 @@ static function (string $callbackId, $stream) use (&$status, $deferred, $stdin, \fclose($stream); } - self::asyncWaitPid($shellPid); + self::asyncWaitPid($proc, $shellPid); }, )); } @@ -75,18 +75,28 @@ public function unreference(): void } } - private static function asyncWaitPid(int $pid): void + /** @param resource $proc */ + private static function asyncWaitPid($proc, int $pid): void { - if (self::hasChildExited($pid)) { + if (self::hasChildExited($proc, $pid)) { return; } - EventLoop::unreference(EventLoop::defer(static fn () => self::asyncWaitPid($pid))); + EventLoop::unreference(EventLoop::defer(static fn () => self::asyncWaitPid($proc, $pid))); } - private static function hasChildExited(int $pid): bool + /** @param resource $proc */ + private static function hasChildExited($proc, int $pid): bool { - return !\extension_loaded('pcntl') || \pcntl_waitpid($pid, $status, \WNOHANG) !== 0; + if (!\function_exists('pcntl_waitpid')) { + return !\proc_get_status($proc)['running']; + } + + do { + $result = \pcntl_waitpid($pid, $status, \WNOHANG); + } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR); + + return $result !== 0; } public function __destruct() @@ -96,18 +106,33 @@ public function __destruct() $this->extraDataPipeCallbackId = null; } - if ($this->joinDeferred->isComplete()) { + if ($this->status === ProcessStatus::Ended) { + $this->reapShell(); + return; + } + + self::asyncWaitPid($this->proc, $this->shellPid); + } + + public function reapShell(): void + { + if (\function_exists('pcntl_waitpid')) { + do { + $result = \pcntl_waitpid($this->shellPid, $status); + } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR); + return; } - self::asyncWaitPid($this->shellPid); + while (\proc_get_status($this->proc)['running']) { + \usleep(1_000); + } } #[\Override] public function wait(): void { - if (\extension_loaded('pcntl')) { - \pcntl_waitpid($this->pid, $status); - } + // Do not block the shutdown handler before ProcHolder destruction terminates the process. + self::hasChildExited($this->proc, $this->shellPid); } } diff --git a/src/Internal/Posix/PosixRunner.php b/src/Internal/Posix/PosixRunner.php index 349b4a0..46c0b84 100644 --- a/src/Internal/Posix/PosixRunner.php +++ b/src/Internal/Posix/PosixRunner.php @@ -22,6 +22,8 @@ */ final class PosixRunner implements ProcessRunner { + private const ESRCH = 3; + use ForbidCloning; use ForbidSerialization; @@ -173,13 +175,32 @@ public function kill(ProcessHandle $handle): void $handle->reference(); $this->signal($handle, 9); + $handle->reapShell(); } #[\Override] public function signal(ProcessHandle $handle, int $signal): void { /** @noinspection PhpComposerExtensionStubsInspection */ - \posix_kill($handle->pid, $signal); + if (\posix_kill($handle->pid, $signal)) { + return; + } + + $error = \posix_get_last_error(); + if ($error === self::ESRCH) { + return; + } + + throw new ProcessException( + \sprintf( + "Failed to send signal %d to process %d: Errno: %d; %s", + $signal, + $handle->pid, + $error, + \posix_strerror($error), + ), + $error, + ); } #[\Override] diff --git a/src/Internal/ProcessHandle.php b/src/Internal/ProcessHandle.php index a25cfb4..b93a133 100644 --- a/src/Internal/ProcessHandle.php +++ b/src/Internal/ProcessHandle.php @@ -14,9 +14,8 @@ abstract class ProcessHandle /** * @var resource - * @psalm-suppress UnusedProperty */ - private $proc; + protected $proc; /** @var DeferredFuture */ public readonly DeferredFuture $joinDeferred; diff --git a/test/ProcessTest.php b/test/ProcessTest.php index 92072a5..a8958ed 100644 --- a/test/ProcessTest.php +++ b/test/ProcessTest.php @@ -6,6 +6,7 @@ use Amp\Future; use Amp\PHPUnit\AsyncTestCase; use Amp\Process\Process; +use Amp\Process\ProcessException; use Amp\TimeoutCancellation; use const Amp\Process\IS_WINDOWS; use function Amp\async; @@ -143,6 +144,123 @@ public function testKillImmediately(): void self::assertSame(IS_WINDOWS ? 1 : 137, $process->join()); } + /** + * @requires extension pcntl + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testKillReapsShellWithPcntlWhileProcessIsRetained(): void + { + $process = Process::start(self::CMD_PROCESS_SLOW); + $process->kill(); + + // Keep Process reachable after kill; this is the state that previously left the shell unreaped. + $status = 0; + $remainingChildPid = \pcntl_waitpid(-1, $status); + $error = \pcntl_get_last_error(); + $exitCode = $process->join(); + + self::assertSame(-1, $remainingChildPid); + self::assertSame(\PCNTL_ECHILD, $error); + self::assertSame(137, $exitCode); + } + + public function testKillReapsShellWithoutPcntlWhileProcessIsRetained(): void + { + if (\DIRECTORY_SEPARATOR === "\\") { + self::markTestSkipped("Signals are not supported on Windows"); + } + + $code = \sprintf( + 'require %s;' + . '$process = Amp\\Process\\Process::start("sleep 30");' + . '$handle = (new ReflectionProperty($process, "handle"))->getValue($process);' + . '$shellPid = (new ReflectionProperty($handle, "shellPid"))->getValue($handle);' + . '$process->kill();' + . 'exit(posix_kill($shellPid, 0) ? 1 : 0);', + \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true), + ); + $process = Process::start([ + \PHP_BINARY, + '-d', + 'disable_functions=pcntl_waitpid,pcntl_get_last_error', + '-r', + $code, + ]); + + $exitCode = $process->join(new TimeoutCancellation(2)); + $error = buffer($process->getStderr()); + + self::assertSame(0, $exitCode, $error); + } + + /** + * @requires extension pcntl + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testProcessDestructionReapsShell(): void + { + $process = Process::start(self::CMD_PROCESS_SLOW); + unset($process); + + $status = 0; + self::assertSame(-1, \pcntl_waitpid(-1, $status, \WNOHANG)); + self::assertSame(\PCNTL_ECHILD, \pcntl_get_last_error()); + } + + /** + * @requires extension pcntl + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testCompletedProcessDestructionReapsShell(): void + { + $process = Process::start('exit 0'); + self::assertSame(0, $process->join()); + unset($process); + + $status = 0; + self::assertSame(-1, \pcntl_waitpid(-1, $status, \WNOHANG)); + self::assertSame(\PCNTL_ECHILD, \pcntl_get_last_error()); + } + + /** + * @requires extension pcntl + */ + public function testShutdownDoesNotWaitForRunningProcess(): void + { + $code = \sprintf( + 'require %s; $GLOBALS["process"] = Amp\\Process\\Process::start("sleep 30");', + \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true), + ); + $process = Process::start([\PHP_BINARY, '-r', $code]); + + self::assertSame(0, $process->join(new TimeoutCancellation(2))); + } + + /** + * @requires extension pcntl + */ + public function testRunningHandleDestructionDoesNotWaitForProcess(): void + { + // Emulate handle destruction in a long-running PHP worker without depending on garbage collection order. + $code = \sprintf( + 'require %s;' + . '$process = Amp\\Process\\Process::start("cat >/dev/null");' + . '$handle = (new ReflectionProperty($process, "handle"))->getValue($process);' + . '$handle->__destruct();', + \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true), + ); + $process = Process::start([\PHP_BINARY, '-r', $code]); + + try { + self::assertSame(0, $process->join(new TimeoutCancellation(2))); + } finally { + $process->kill(); + } + } + public function testKillThenReadStdout(): void { $this->setTimeout(1); @@ -259,6 +377,42 @@ public function testSignal(): void self::assertSame(42, $process->join()); } + /** + * @requires extension posix + */ + public function testSignalThrowsIfDeliveryFails(): void + { + $process = Process::start(self::CMD_PROCESS_SLOW); + + try { + $this->expectException(ProcessException::class); + $this->expectExceptionMessage('Failed to send signal 9999'); + $process->signal(9999); + } finally { + $process->kill(); + $process->join(); + } + } + + /** + * @requires extension posix + */ + public function testSignalIgnoresProcessThatAlreadyExited(): void + { + $process = Process::start('exit 0'); + + // Keep the event loop paused so Process status is not updated before the OS process exits. + $isRunning = true; + for ($attempt = 0; $attempt < 1000 && $isRunning; ++$attempt) { + $isRunning = \posix_kill($process->getPid(), 0); + \usleep(1000); + } + + self::assertFalse($isRunning); + $process->signal(0); + self::assertSame(0, $process->join()); + } + public function testCancellation(): void { $this->expectException(CancelledException::class);