-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
PHP gRPC extension breaks use of --parallel
#294
Comments
Tagging this ticket with "Help wanted" as I cannot triage/test this, nor validate a fix (parallel is not supported on Windows). |
I use WSL2 on Windows. Parallel works in WSL. |
--parallel
--parallel
As a suggestion for a fix, maybe something like this? ...
// If the PCNTL extension isn't installed, we can't fork.
if (function_exists('pcntl_fork') === false) {
$this->config->parallel = 1;
}
// If the gRPC extension settings disabled forking, we can't fork.
if (!ini_get('grpc.enable_fork_support') || !ini_get('grpc.poll_strategy')) {
$this->config->parallel = 1;
} |
@mbomb007 I don't think that fix suggestion is correct as |
So maybe this, then? // If the gRPC extension settings disabled forking, we can't fork.
if (extension_loaded('grpc') && (!ini_get('grpc.enable_fork_support') || !ini_get('grpc.poll_strategy'))) {
$this->config->parallel = 1;
} |
@mbomb007 I'd like to see at least one other person confirming the issue + confirmation of the fix; or alternatively proof of the issue via a test, which could also safeguard the fix. Regarding the fix itself, please use more exact checks, i.e. not As the fix would need to go in the For reference for others coming across this issue:
Also, while trying to find out whether these ini settings are |
I've installed the grpc extension via pecl on my mac machine (running PHP 8.1.25), and can confirm that when you run the phpcs with So I went and checked what could be stalling so I played with verbosity flags, and phpcs --standard=psr12 --parallel=2 src -v
Registering sniffs in the PSR12 standard... DONE (60 sniffs registered)
Creating file list... DONE (3 files in queue) And stalled. But running either with |
Those flags do indeed disregard the |
Personally I'd consider setting the required grpc-INI settings a responsibility of the user, if they want to use ext-grpc together with ext-pcntl. Anyway, here's an example of how Psalm deals with it by disabling ext-grpc and restarting the process with the help of |
The phpcs project is responsible for fixing the hangs, IMHO. Because people who use phpcs might have no idea that they have The code already checks if the fork function exists. It should do the same for the settings, to make sure users don't experience hangs. // If the PCNTL extension isn't installed, we can't fork.
if (function_exists('pcntl_fork') === false) {
$this->config->parallel = 1;
} Anyway, the Psalm code does show how they check for it: extension_loaded('grpc') && (ini_get('grpc.enable_fork_support') === '1' && ini_get('grpc.poll_strategy') === 'epoll1') === false This would also be exactly the same: extension_loaded('grpc') && (ini_get('grpc.enable_fork_support') !== '1' || ini_get('grpc.poll_strategy') !== 'epoll1') |
I can make a PR, but what do we think should happen? Personally, I think setting |
I don't necessarily agree. PHPCS checking for In that regards, it might be more correct to have a check in the |
All they say on grpc is:
Originally posted by @stanley-cheung in grpc/grpc#20250 (comment) |
Well, you have the ability to prevent a hang or exit instead of hang. I think that would be preferrable. |
The Docker image I use did just fix their gRPC issue, but that doesn't mean this won't occur for anyone else. |
@mbomb007 Thanks for the update. In that case, I suggest closing this ticket. I did some digging and over the years, there have been a handful of issues reported which alluded to the use of gRPC being problematic. phpcs -d grpc.enable_fork_support=1 -d grpc.poll_strategy=epoll1 ...other options... Or not using the A handful or reports in relation to a broken PHP extension is not a good justification for adding a "fix"/work-around in PHPCS itself. Especially if there are other workable solutions available. If there would be more frequent reports of this being an issue, adding the work-around could be revisited, but I maintain that preferably the issue should be fixed in gRPC itself. For the record/future reference, these are the older issues I could find:
I also came across some other external references. I will update the links in my previous comment to add them so those lists are more complete. |
I certainly don't expect PHPCS to fix gRPC, but given that you know this is causing users pain, I think a simple check and warning would be very kind and appreciated. I'm so lucky to have stumbled across this issue. PHPCS started stalling out today for no reason I could think of, and it was only after digging through months of GitHub issues that I saw gRPC in the title of this one and the lightbulb clicked: I had installed gRPC as a requirement for an unrelated project. I could have spent hours on this and possibly never resolved it at all without this discovery. |
Oh wow. I just discovered this is actually the second time I've been bitten by this bug and I didn't even remember. Here's the same PHPCS issue I posted 5 years ago: squizlabs/PHP_CodeSniffer#2767 |
Describe the bug
A clear and concise description of what the bug is.
See grpc/grpc#20250
To reproduce
Steps to reproduce the behavior:
The defaults are
--parallel=2
. It will hang.Expected behavior
It would be helpful if src/Runner.php would check if the extension exists and either
Or, if it detects that fork support is disabled, emit a warning and set
--parallel=1
.Versions (please complete the following information)
wodby/drupal-php:8.3-dev-4.52.0
]Additional context
Add any other context about the problem here.
Related: #10
Parent issue: wodby/drupal-php#102
Please confirm:
master
branch of PHP_CodeSniffer.The text was updated successfully, but these errors were encountered: