Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw exception instead of silently logging issues occurred during scan #10902

Merged
merged 5 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Psalm/Internal/Codebase/Analyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -511,10 +511,6 @@ static function (): void {
$this->argument_map[$file_path] = $argument_map;
}
}

if ($pool->didHaveError()) {
exit(1);
}
} else {
$i = 0;

Expand Down
4 changes: 0 additions & 4 deletions src/Psalm/Internal/Codebase/Scanner.php
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,6 @@ function () {
);
}
}

if ($pool->didHaveError()) {
exit(1);
}
} else {
$i = 0;

Expand Down
52 changes: 24 additions & 28 deletions src/Psalm/Internal/Fork/Pool.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,6 @@ final class Pool
/** @var resource[] */
private array $read_streams = [];

private bool $did_have_error = false;

/** @var ?Closure(mixed): void */
private ?Closure $task_done_closure = null;

Expand Down Expand Up @@ -297,6 +295,20 @@ private static function streamForChild(array $sockets)
return $for_write;
}

private function killAllChildren(): void
{
foreach ($this->child_pid_list as $child_pid) {
/**
* SIGTERM does not exist on windows
*
* @psalm-suppress UnusedPsalmSuppress
* @psalm-suppress UndefinedConstant
* @psalm-suppress MixedArgument
*/
posix_kill($child_pid, SIGTERM);
}
}

/**
* Read the results that each child process has serialized on their write streams.
* The results are returned in an array, one for each worker. The order of the results
Expand All @@ -319,6 +331,7 @@ private function readResultsFromChildren(): array
$content = array_fill_keys(array_keys($streams), '');

$terminationMessages = [];
$done = [];

// Read the data off of all the stream.
while (count($streams) > 0) {
Expand Down Expand Up @@ -361,34 +374,25 @@ private function readResultsFromChildren(): array
if ($message instanceof ForkProcessDoneMessage) {
$terminationMessages[] = $message->data;
} elseif ($message instanceof ForkTaskDoneMessage) {
$done[(int)$file] = true;
if ($this->task_done_closure !== null) {
($this->task_done_closure)($message->data);
}
} elseif ($message instanceof ForkProcessErrorMessage) {
// Kill all children
foreach ($this->child_pid_list as $child_pid) {
/**
* SIGTERM does not exist on windows
*
* @psalm-suppress UnusedPsalmSuppress
* @psalm-suppress UndefinedConstant
* @psalm-suppress MixedArgument
*/
posix_kill($child_pid, SIGTERM);
}
$this->killAllChildren();
throw new Exception($message->message);
} else {
error_log('Child should return ForkMessage - response type=' . gettype($message));
$this->did_have_error = true;
$this->killAllChildren();
throw new Exception('Child should return ForkMessage - response type=' . gettype($message));
}
}
}

// If the stream has closed, stop trying to select on it.
if (feof($file)) {
if ($content[(int)$file] !== '') {
error_log('Child did not send full message before closing the connection');
$this->did_have_error = true;
if ($content[(int)$file] !== '' || !isset($done[(int)$file])) {
$this->killAllChildren();
throw new Exception('Child did not send full message before closing the connection');
}

fclose($file);
Expand Down Expand Up @@ -450,21 +454,13 @@ public function wait(): array
* @psalm-suppress UndefinedConstant
*/
if ($term_sig !== SIGALRM) {
$this->did_have_error = true;
error_log("Child terminated with return code $return_code and signal $term_sig");
$this->killAllChildren();
throw new Exception("Child terminated with return code $return_code and signal $term_sig");
}
}
}
}

return $content;
}

/**
* Returns true if this had an error, e.g. due to memory limits or due to a child process crashing.
*/
public function didHaveError(): bool
{
return $this->did_have_error;
}
}
5 changes: 4 additions & 1 deletion src/Psalm/Internal/Fork/PsalmRestarter.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use function array_filter;
use function array_merge;
use function array_splice;
use function assert;
use function count;
use function extension_loaded;
use function file_get_contents;
use function file_put_contents;
Expand Down Expand Up @@ -125,7 +127,7 @@ private static function toBytes(string $value): int
/**
* No type hint to allow xdebug-handler v1 and v2 usage
*
* @param string[] $command
* @param non-empty-list<string> $command
* @phpcsSuppress SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint
*/
protected function restart($command): void
Expand Down Expand Up @@ -167,6 +169,7 @@ protected function restart($command): void
0,
$additional_options,
);
assert(count($command) > 1);

parent::restart($command);
}
Expand Down