From c89a4a3d63b61402bcce5c35ca3f8e64087f3a75 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Tue, 3 Nov 2026 17:32:24 +0000 Subject: [PATCH] Spawn the PHPStan Pro worker instead of forking it FixerApplication runs for as long as PHPStan Pro does. A fixer:worker forked from it inherited the container and the config it started with, so an edit of a config file, like errors added to the baseline, was noticed by the file monitor but the reanalysis still used the old ignoreErrors and parameters. A spawned fixer:worker reads the config files again on every analysis. It still forks its own analysis workers where that is supported, see ParallelAnalyser. The validation of the ignoreErrors in FixerApplication is gone too: it checked the config the process started with. fixer:worker validates the current one. Amp-Thread-ID: https://ampcode.com/threads/T-01a1a198-fa57-7583-9394-63372a7e2fa8 --- src/Command/FixerApplication.php | 84 ++-------- src/Command/FixerWorkerCommand.php | 3 - src/Command/FixerWorkerRunner.php | 10 +- src/Process/ForkedProcessPromise.php | 231 -------------------------- src/Process/ProcessPromise.php | 24 --- src/Process/SpawnedProcessPromise.php | 9 +- 6 files changed, 20 insertions(+), 341 deletions(-) delete mode 100644 src/Process/ForkedProcessPromise.php delete mode 100644 src/Process/ProcessPromise.php diff --git a/src/Command/FixerApplication.php b/src/Command/FixerApplication.php index 2ebe31762bd..a98b7fa1d12 100644 --- a/src/Command/FixerApplication.php +++ b/src/Command/FixerApplication.php @@ -11,7 +11,6 @@ use GuzzleHttp\RequestOptions; use Nette\Utils\Json; use Phar; -use PHPStan\Analyser\Ignore\IgnoredErrorHelper; use PHPStan\Analyser\InternalError; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; @@ -19,18 +18,14 @@ use PHPStan\File\FileMonitorResult; use PHPStan\File\FileReader; use PHPStan\File\FileWriter; -use PHPStan\File\PathNotFoundException; use PHPStan\Internal\ComposerHelper; use PHPStan\Internal\DirectoryCreator; use PHPStan\Internal\DirectoryCreatorException; use PHPStan\Internal\HttpClientFactory; -use PHPStan\Parallel\ForkParallelChecker; use PHPStan\PhpDoc\StubFilesProvider; -use PHPStan\Process\ForkedProcessPromise; use PHPStan\Process\ProcessCanceledException; use PHPStan\Process\ProcessCrashedException; use PHPStan\Process\ProcessHelper; -use PHPStan\Process\ProcessPromise; use PHPStan\Process\SpawnedProcessPromise; use PHPStan\ShouldNotHappenException; use React\ChildProcess\Process; @@ -76,7 +71,6 @@ final class FixerApplication */ public function __construct( private FileMonitor $fileMonitor, - private IgnoredErrorHelper $ignoredErrorHelper, private StubFilesProvider $stubFilesProvider, #[AutowiredParameter] private array $analysedPaths, @@ -97,8 +91,6 @@ public function __construct( #[AutowiredParameter] private string $usedLevel, private HttpClientFactory $httpClientFactory, - private ForkParallelChecker $forkParallelChecker, - private FixerWorkerRunner $fixerWorkerRunner, ) { } @@ -121,7 +113,7 @@ public function run( /** @var int<0, 65535> $serverPort */ $serverPort = parse_url($serverAddress, PHP_URL_PORT); - $server->on('connection', function (ConnectionInterface $connection) use ($loop, $inceptionResult, $projectConfigFile, $input, $output, $mainScript, $filesCount): void { + $server->on('connection', function (ConnectionInterface $connection) use ($loop, $projectConfigFile, $input, $output, $mainScript, $filesCount): void { // phpcs:disable SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly $jsonInvalidUtf8Ignore = defined('JSON_INVALID_UTF8_IGNORE') ? JSON_INVALID_UTF8_IGNORE : 0; // phpcs:enable @@ -164,14 +156,13 @@ public function run( $this->analyse( $loop, - $inceptionResult, $mainScript, $projectConfigFile, $input, $encoder, ); - $this->monitorFileChanges($loop, function (FileMonitorResult $changes) use ($loop, $inceptionResult, $mainScript, $projectConfigFile, $input, $encoder): void { + $this->monitorFileChanges($loop, function (FileMonitorResult $changes) use ($loop, $mainScript, $projectConfigFile, $input, $encoder): void { if ($this->processInProgress !== null) { $this->processInProgress->cancel(); $this->processInProgress = null; @@ -185,7 +176,6 @@ public function run( $this->analyse( $loop, - $inceptionResult, $mainScript, $projectConfigFile, $input, @@ -424,18 +414,12 @@ private function monitorFileChanges(LoopInterface $loop, callable $hasChangesCal private function analyse( LoopInterface $loop, - InceptionResult $inceptionResult, string $mainScript, ?string $projectConfigFile, InputInterface $input, Encoder $phpstanFixerEncoder, ): void { - $ignoredErrorHelperResult = $this->ignoredErrorHelper->initialize(); - if (count($ignoredErrorHelperResult->getErrors()) > 0) { - throw new ShouldNotHappenException(); - } - // TCP server for fixer:worker (TCP client) $server = new TcpServer('127.0.0.1:0', $loop); /** @var string $serverAddress */ @@ -453,16 +437,20 @@ private function analyse( }); }); - $process = $this->createProcessPromise( - $this->forkParallelChecker->isSupported(), - $loop, - $server, + // Always a fresh process, never a fork of this one: this process runs as long as + // PHPStan Pro does, so its container and config are the ones it started with. + // A spawned fixer:worker reads the config files again, which picks up edits + // like errors added to the baseline. It still forks its own analysis workers. + $process = new SpawnedProcessPromise($loop, ProcessHelper::getWorkerCommand( $mainScript, + 'fixer:worker', $projectConfigFile, + [ + '--server-port', + (string) $serverPort, + ], $input, - $serverPort, - $inceptionResult, - ); + )); $this->processInProgress = $process->run(); $this->processInProgress->then(function () use ($server): void { @@ -573,50 +561,4 @@ private function getStubFiles(): array return $stubFiles; } - /** - * @param int<0, 65535> $serverPort - */ - private function createProcessPromise( - bool $useFork, - LoopInterface $loop, - TcpServer $server, - string $mainScript, - ?string $projectConfigFile, - InputInterface $input, - int $serverPort, - InceptionResult $inceptionResult, - ): ProcessPromise - { - if ($useFork) { - try { - [$inceptionFiles, $isOnlyFiles] = $inceptionResult->getFiles(); - } catch (PathNotFoundException | InceptionNotSuccessfulException) { - throw new ShouldNotHappenException(); - } - - return new ForkedProcessPromise( - $loop, - $this->fixerWorkerRunner, - $server, - $inceptionFiles, - $isOnlyFiles, - $inceptionResult->getProjectConfigArray(), - $projectConfigFile, - $serverPort, - $input, - ); - } - - return new SpawnedProcessPromise($loop, ProcessHelper::getWorkerCommand( - $mainScript, - 'fixer:worker', - $projectConfigFile, - [ - '--server-port', - (string) $serverPort, - ], - $input, - )); - } - } diff --git a/src/Command/FixerWorkerCommand.php b/src/Command/FixerWorkerCommand.php index 60357599096..0461df1dc31 100644 --- a/src/Command/FixerWorkerCommand.php +++ b/src/Command/FixerWorkerCommand.php @@ -98,9 +98,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int throw new ShouldNotHappenException(); } - // Everything after the boot lives in FixerWorkerRunner so a - // pcntl_fork()-ed child can reuse it without re-booting (see - // FixerApplication). $fixerWorkerRunner = $container->getByType(FixerWorkerRunner::class); return $fixerWorkerRunner->run( diff --git a/src/Command/FixerWorkerRunner.php b/src/Command/FixerWorkerRunner.php index 182fb125d9d..f0205dda5da 100644 --- a/src/Command/FixerWorkerRunner.php +++ b/src/Command/FixerWorkerRunner.php @@ -39,11 +39,9 @@ /** * The PHPStan Pro worker logic that runs *after* the application boot. * - * Extracted from FixerWorkerCommand so it can be reused without re-booting: a - * pcntl_fork()-ed child (see ForkedProcessPromise) inherits the already-booted - * DI container and calls run() directly, while FixerWorkerCommand still calls - * it after the expensive CommandHelper::begin() boot of a freshly spawned - * process. + * FixerWorkerCommand calls it after the CommandHelper::begin() boot of the + * process FixerApplication spawns for each analysis. The analysis itself + * forks its workers where that is supported (see ParallelAnalyser). * * It connects back to FixerApplication's TCP server, restores the result * cache, analyses the changed files and streams the results. @@ -82,8 +80,6 @@ public function run( throw new ShouldNotHappenException(); } - // Always a fresh event loop: in a forked child the parent's inherited - // loop must never be touched. $loop = new StreamSelectLoop(); $tcpConnector = new TcpConnector($loop); $tcpConnector->connect(sprintf('127.0.0.1:%d', $serverPort))->then(function (ConnectionInterface $connection) use ($errorOutput, $inceptionFiles, $isOnlyFiles, $projectConfigArray, $configuration, $input, $ignoredErrorHelperResult, $loop): void { diff --git a/src/Process/ForkedProcessPromise.php b/src/Process/ForkedProcessPromise.php deleted file mode 100644 index 2f4158d77cc..00000000000 --- a/src/Process/ForkedProcessPromise.php +++ /dev/null @@ -1,231 +0,0 @@ - */ - private Deferred $deferred; - - private ?int $childPid = null; - - /** @var resource|null */ - private $stdOut = null; - - /** @var resource|null */ - private $stdErr = null; - - private ?TimerInterface $waitTimer = null; - - private bool $canceled = false; - - /** - * @param string[] $inceptionFiles - * @param mixed[]|null $projectConfigArray - */ - public function __construct( - private LoopInterface $loop, - private FixerWorkerRunner $fixerWorkerRunner, - private TcpServer $server, - private array $inceptionFiles, - private bool $isOnlyFiles, - private ?array $projectConfigArray, - private ?string $configuration, - private int $serverPort, - private InputInterface $input, - ) - { - $this->deferred = new Deferred(function (): void { - $this->cancel(); - }); - } - - /** - * @return PromiseInterface - */ - public function run(): PromiseInterface - { - // Created before the fork so the parent can read what the child wrote. - $tmpStdOut = tmpfile(); - if ($tmpStdOut === false) { - throw new ShouldNotHappenException('Failed creating temp file for stdout.'); - } - $tmpStdErr = tmpfile(); - if ($tmpStdErr === false) { - fclose($tmpStdOut); - throw new ShouldNotHappenException('Failed creating temp file for stderr.'); - } - $this->stdOut = $tmpStdOut; - $this->stdErr = $tmpStdErr; - - $pid = pcntl_fork(); - - if ($pid === -1) { - $this->closeCaptureFiles(); - // Deferred so it runs after FixerApplication has stored the promise. - $this->loop->futureTick(function (): void { - $this->deferred->reject(new ProcessCrashedException('pcntl_fork() failed.')); - }); - - return $this->deferred->promise(); - } - - if ($pid === 0) { - // Child: drop the inherited listening socket immediately, then run - // the worker on its own fresh event loop and never return. - $this->server->close(); - ForkedChildCrashReporter::install($tmpStdErr); - // after the crash reporter: its shutdown function must run first - ForkedChildTerminator::install(); - // The worker writes its errors into the capture the parent reads, - // like a spawned worker whose stderr is the capture itself. - $childErrorStream = new StreamOutput($tmpStdErr); - $exitCode = $this->fixerWorkerRunner->run( - new SymfonyOutput($childErrorStream, new SymfonyStyle(new ErrorsConsoleStyle(new StringInput(''), $childErrorStream))), - $this->inceptionFiles, - $this->isOnlyFiles, - $this->projectConfigArray, - $this->configuration, - $this->serverPort, - $this->input, - ); - exit($exitCode); - } - - // Parent: poll for the child to exit and resolve/reject accordingly. - $this->childPid = $pid; - $this->waitTimer = $this->loop->addPeriodicTimer(self::WAITPID_POLL_INTERVAL, function () use ($pid): void { - $status = 0; - $result = pcntl_waitpid($pid, $status, WNOHANG); - if ($result === 0) { - return; - } - - $this->cancelWaitTimer(); - - $stdOut = ''; - if ($this->stdOut !== null) { - rewind($this->stdOut); - $stdOut = (string) stream_get_contents($this->stdOut); - } - $stdErr = ''; - if ($this->stdErr !== null) { - rewind($this->stdErr); - $stdErr = (string) stream_get_contents($this->stdErr); - } - $this->closeCaptureFiles(); - - if ($this->canceled) { - // cancel() already rejected the promise; just reap the child. - return; - } - - $exitCode = null; - if ($result > 0 && pcntl_wifexited($status)) { - $exitStatus = pcntl_wexitstatus($status); - if ($exitStatus !== false) { - $exitCode = $exitStatus; - } - } - - if ($exitCode === 0) { - $this->deferred->resolve($stdOut); - return; - } - - // A child killed by a signal writes nothing of its own, so naming - // the signal is all the crash report there is. - if ($result > 0 && pcntl_wifsignaled($status)) { - $signal = pcntl_wtermsig($status); - if ($signal !== false) { - $this->deferred->reject(new ProcessCrashedException(sprintf( - "Child process was killed by signal %s.\n%s", - TerminationSignal::describe($signal), - $stdOut . $stdErr, - ))); - return; - } - } - - $this->deferred->reject(new ProcessCrashedException($stdOut . $stdErr)); - }); - - return $this->deferred->promise(); - } - - private function cancel(): void - { - if ($this->childPid === null) { - throw new ShouldNotHappenException('Cancelling process before running'); - } - $this->canceled = true; - // SIGTERM the child; the waitpid poll timer keeps running so it still - // gets reaped (otherwise: zombie). - posix_kill($this->childPid, SIGTERM); - $this->deferred->reject(new ProcessCanceledException()); - } - - private function cancelWaitTimer(): void - { - if ($this->waitTimer === null) { - return; - } - - $this->loop->cancelTimer($this->waitTimer); - $this->waitTimer = null; - } - - private function closeCaptureFiles(): void - { - if ($this->stdOut !== null) { - fclose($this->stdOut); - $this->stdOut = null; - } - if ($this->stdErr === null) { - return; - } - - fclose($this->stdErr); - $this->stdErr = null; - } - -} diff --git a/src/Process/ProcessPromise.php b/src/Process/ProcessPromise.php deleted file mode 100644 index 1d588c42073..00000000000 --- a/src/Process/ProcessPromise.php +++ /dev/null @@ -1,24 +0,0 @@ - - */ - public function run(): PromiseInterface; - -} diff --git a/src/Process/SpawnedProcessPromise.php b/src/Process/SpawnedProcessPromise.php index 4504e4161d0..5530080913d 100644 --- a/src/Process/SpawnedProcessPromise.php +++ b/src/Process/SpawnedProcessPromise.php @@ -13,12 +13,11 @@ use function tmpfile; /** - * ProcessPromise backed by a freshly spawned PHP process (react/child-process). - * - * @see ForkedProcessPromise for the pcntl_fork()-based alternative that skips - * the per-worker application re-boot. + * A freshly spawned PHP process (react/child-process) as a promise that resolves + * with its output on success and rejects with ProcessCrashedException or + * ProcessCanceledException otherwise. */ -final class SpawnedProcessPromise implements ProcessPromise +final class SpawnedProcessPromise { /** @var Deferred */