PHP gRPC extension breaks use of --parallel

[mbomb007]

Describe the bug

A clear and concise description of what the bug is.

See grpc/grpc#20250

To reproduce

Steps to reproduce the behavior:

1. Add the grpc PHP extension

test=$(php -r "echo ini_get('grpc.enable_fork_support');")
echo "grpc.enable_fork_support: $test"
test=$(php -r "echo ini_get('grpc.poll_strategy');")
echo "grpc.poll_strategy: $test"

The defaults are

grpc.enable_fork_support = 0
grpc.poll_strategy = 

3. Run PHPCS using --parallel=2. It will hang.

Expected behavior

It would be helpful if src/Runner.php would check if the extension exists and either

ini_set('grpc.enable_fork_support', true);
ini_set('grpc.poll_strategy', 'epoll1');

Or, if it detects that fork support is disabled, emit a warning and set --parallel=1.

Versions (please complete the following information)

Operating System	[Docker wodby/drupal-php:8.3-dev-4.52.0]
PHP version	[8.3]
PHP_CodeSniffer version	[3.8.1]
Standard	[Drupal,DrupalPractice]
Install type	[Composer]

Additional context

Add any other context about the problem here.

Related: #10

Parent issue: wodby/drupal-php#102

Please confirm:

• I have searched the issue list and am not opening a duplicate issue.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[jrfnl]

Tagging this ticket with "Help wanted" as I cannot triage/test this, nor validate a fix (parallel is not supported on Windows).

[mbomb007]

I use WSL2 on Windows. Parallel works in WSL.

[mbomb007]

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;
        }

[jrfnl]

@mbomb007 I don't think that fix suggestion is correct as ini_get() will return false if the configuration option doesn't exist and I suspect it will not exist in the majority of cases, as gRPC is a PECL extension which will only be installed by a limited set of people.

[mbomb007]

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;
        }

[jrfnl]

@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 !ini_get('grpc.enable_fork_support'), but something like ini_get('grpc.enable_fork_support') !== '1'.

As the fix would need to go in the Runner class, this might be a good reason to prioritize end-to-end tests (see #147).

For reference for others coming across this issue:

• PHP gRPC extension breaks fork functions grpc/grpc#20250
• gRPC PHP extension prevents pcntl_waitpid() to return PID grpc/grpc#20994
• https://grpc.github.io/grpc/php/md_src_php__r_e_a_d_m_e.html
• https://pecl.php.net/package/grpc
• https://grpc.io/docs/languages/php/

Also, while trying to find out whether these ini settings are PHP_INI_SYSTEM, PHP_INI_PERDIR, PHP_INI_USER or PHP_INI_ALL, I came across the following issues about gRPC and forked processes which look loosely related:

• PHP gRPC extension breaks fork functions grpc/grpc#13412
• gRPC extension prevents PHP execution grpc/grpc#31772
• https://bugs.php.net/bug.php?id=78717

[dingo-d]

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 parallel option it stalls.

So I went and checked what could be stalling so I played with verbosity flags, and -v gave me:

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 -vv or -vvv worked. Not sure if those flags are maybe disregarding the parallel option 🤷🏼‍♂️

[mbomb007]

But running either with -vv or -vvv worked. Not sure if those flags are maybe disregarding the parallel option 🤷🏼‍♂️

Those flags do indeed disregard the parallel option.

[tscni]

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.
Instead of having every PHP tool with fork based parallelism deal with this, shouldn't it rather be on ext-grpc to highlight this issue more strongly or change the defaults? (but ultimately I suppose it's a way to avoid unnecessary support requests)

Anyway, here's an example of how Psalm deals with it by disabling ext-grpc and restarting the process with the help of composer/xdebug-handler: https://github.com/vimeo/psalm/blob/541bf51a2534a00d8bba5a6b76542f0c5a84a33d/src/Psalm/Internal/Cli/Psalm.php#L879-L890

[mbomb007]

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.

The phpcs project is responsible for fixing the hangs, IMHO. Because people who use phpcs might have no idea that they have grpc. I don't use or know anything about that extension; it just happens to be a dependency of the wodby/php Docker image. I do, however, expect phpcs to work without hanging.

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')

[mbomb007]

I can make a PR, but what do we think should happen? Personally, I think setting parallel=1 is fine.

[jrfnl]

The phpcs project is responsible for fixing the hangs, IMHO.
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.

I don't necessarily agree. PHPCS checking for function_exists('pcntl_fork') is valid as that is functionality PHPCS itself will call.
This is different from checking for the gRPC extension and gRPC ini settings, as PHPCS does not use that functionality at all.
To be honest, I agree with @tscni here and regard this as a bug in the gRPC project, which should be fixed there.

In that regards, it might be more correct to have a check in the Runner::checkRequirements() method and bow out completely if the gRPC extension is enabled.
The Runner::checkRequirements() method, however, runs too early to only bow out selectively only when the parallel option is enabled, so the currently proposed solution would be more user friendly.

[mbomb007]

All they say on grpc is:

Fork support was not implemented across language uniformly at the moment. When it is, we will consider making that behavior the default.

Originally posted by @stanley-cheung in grpc/grpc#20250 (comment)

[mbomb007]

Well, you have the ability to prevent a hang or exit instead of hang. I think that would be preferrable.

[mbomb007]

The Docker image I use did just fix their gRPC issue, but that doesn't mean this won't occur for anyone else.

[jrfnl]

@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.
However, there has always been a working solution available, which is running PHPCS like so:

phpcs -d grpc.enable_fork_support=1 -d grpc.poll_strategy=epoll1 ...other options...

Or not using the parallel option when gRPC is loaded.

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:

• PHPCS hangs in Docker squizlabs/PHP_CodeSniffer#2767 - this is basically your issue as the OP was also using the Drupal Docker container.
• phpcs hang, except in verbose mode! squizlabs/PHP_CodeSniffer#2983
• PHP_CodeSniffer gets inifinitely stuck with parallel processing enabled if the gRPC extension is loaded squizlabs/PHP_CodeSniffer#3518

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.

[danepowell]

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.

[danepowell]

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

[derrabus]

Hello. I'd like to weigh in on this issue because I've lost a couple of hours of my life on it.

I'd like to introduce PHPCS to a project that uses the GRPC extension and PHPCS just hangs without any output whatsoever. It was by trail and error that I found out that disabling the GRPC extension fixes the problem.

I understand that the root cause of the problem is to be found in the GRPC extension itself. But browsing through the discussions on their tracker, I'm not very confident that this will ever happen. The issue has been present and known for almost a decade.

I also understand that we want to avoid making the PHPCS repo a collection of workarounds for broken PHP extensions, but given that the problem likely won't be solved upstream and given that it is easily detectable before running into it, can we maybe find a nice user-friendly way forward?

[jrfnl]

@derrabus I'm open to suggestions with the above mentioned caveats.

One thing I have in mind for the future (but isn't my current priority, so it may well be a while before the feature exists) is to add a --diagnose option which would check various hard and soft system requirements and report on those.
I have some notes on this locally, but no definitive design for it yet.

I can also imagine just adding a plain warning in the requirements check (as part of #530).

Loosely related issues:

• Docs: add more information about parallel option #10
• Inform user if parallelization is disabled at runtime and why #419, which was fixed via Config: revamp the help screen(s) #447
• 4.0 | Refactor the requirements check #530

[jrfnl]

@derrabus Thinking in the context of a warning, I'd also love to hear your thoughts on the proposal in #184 (comment) and my extra question regarding runtime system warnings/notices etc in #184 (comment)

[derrabus]

@derrabus I'm open to suggestions with the above mentioned caveats.

My favorite solution would be disabling parallel execution and displaying a small warning on why we did that. Would that be feasible?

[jrfnl]

@derrabus I'm open to suggestions with the above mentioned caveats.

My favorite solution would be disabling parallel execution and displaying a small warning on why we did that. Would that be feasible?

@derrabus That has been discussed above already and rejected.

[derrabus]

@derrabus That has been discussed above already and rejected.

Maybe I missed something, but I did not find any reason other than that the issue hasn't been reported frequently enough. I find the approach of disabling parallel execution reasonable, given that this is also the behavior when PCNTL is not available. So why not do the same if we detect that PCNTL is broken?

[jrfnl]

@derrabus

Maybe I missed something....

From #294 (comment):

PHPCS checking for function_exists('pcntl_fork') is valid as that is functionality PHPCS itself will call.
This is different from checking for the gRPC extension and gRPC ini settings, as PHPCS does not use that functionality at all.

[derrabus]

I'm going to assume that quote is meant to answer the question I've ended my previous comment with.

PHPCS checking for function_exists('pcntl_fork') is valid as that is functionality PHPCS itself will call.

Yes, it's a valid way of checking if PCNTL is available.

This is different from checking for the gRPC extension and gRPC ini settings, as PHPCS does not use that functionality at all.

Yes, that is different. Because that check attempts to find out if a call to PCNTL will actually work. Right now, we could easily prevent a call that would 100% cause the PHPCS process to hang without any feedback. It's not the fault of PHPCS that this happens, but we could do something that helps people to not run into a trap.

[jrfnl]

@derrabus That's the same argument which has been discussed above already. It's not a PHPCS bug, but a bug in gRPC and should be solved there. Adding a work-around in PHPCS for a bug in a unrelated tool which is not being used by PHPCS, is not the way and would open the door to all sorts of nonsense "fixes" for system configuration issues outside of the control of PHPCS.

[derrabus]

Juliette, I'm going to bail out of this thread now because you've made up your mind already as it seems. Just for the future, if I'm asking for a way forward and you've already made the decision that there is none, just say so. It'll save the both of us a lot of time.

I think, our assesments of this situation pretty much align, but we draw different conclusions. That's fair and I'll have to accept yours. For my current project, this unfortunately means that PHPCS won't be integrated which I find highly unfortunate.

[jrfnl]

@derrabus As I said before, I'm open to suggestions, but not to rehashing the discussion which was already had above.

If you can think of something which would help or have thoughts about the other suggestions I left in my initial response to you, I'd definitely like to hear them.

[derrabus]

I didn't participate in that discussion back then and found that the arguments back then hadn't been invalidated. The new argument that I could bring to the table is the time that has passed since the original discussion.

I contribute to projects that I care about. GRPC is a project that I don't care much about and tbh it looks like they don't care much about the PHP ecosystem either. A fix in GRPC hasn't happened for years and I personally doubt that it ever will. However, people might be stuck with this extension, especially if they need to operate on Google's cloud infrastructure.

PHPCS is a project I do care about. I want this tool to succeed, I want people to use it. Seeing it fail in certain environments without any feedback whatsoever pains me. Especially when the agreed resolution is to fingerpoint at a badly maintained PHP extension. If we want to give helpful feedback to the user, there's no way around making PHPCS aware of that extension in one way or another.

[jrfnl]

@derrabus

I didn't participate in that discussion back then and found that the arguments back then hadn't been invalidated. The new argument that I could bring to the table is the time that has passed since the original discussion.

Time has passed indeed and the amount of people who reported an issue with this hasn't gone up significantly, so I don't see how that would invalidate the previous discussion.

I contribute to projects that I care about. GRPC is a project that I don't care much about and tbh it looks like they don't care much about the PHP ecosystem either. A fix in GRPC hasn't happened for years and I personally doubt that it ever will. However, people might be stuck with this extension, especially if they need to operate on Google's cloud infrastructure.

Understood, but as I already posted in this thread (and probably in previous threads too), there is a perfectly viable solution available to PHPCS users who run in an environment which includes the gRPC extension:

phpcs -d grpc.enable_fork_support=1 -d grpc.poll_strategy=epoll1 ...other options...

In other words, there is no reason why your current project wouldn't be able to use PHPCS successfully...

PHPCS is a project I do care about. I want this tool to succeed, I want people to use it.

Appreciated.

Seeing it fail in certain environments without any feedback whatsoever pains me.

That's the part where I'm definitely open to suggestions. Again, see my earlier response about this above. I'm also open to adding an FAQ entry to the wiki etc.
Ideas to improve the feedback loop/prevent people wasting time figuring out what's going on are welcome.

However, it does remain the responsibility of the users running this exotic extension to prevent the problem.

Especially when the agreed resolution is to fingerpoint at a badly maintained PHP extension. If we want to give helpful feedback to the user, there's no way around making PHPCS aware of that extension in one way or another.

The problem with that, as I said before, is that it opens the door to adding work-arounds for other things which PHPCS doesn't use.
Aside from that, any such fix would need to be safeguarded and maintained, which means setting up a test environment in CI which includes gRPC, and having to maintain that test environment as well, while gRPC is not used by PHPCS. To me that is convoluted. Especially when there is a working solution available for anyone who runs in an environment with gRPC.

[derrabus]

Time has passed indeed and the amount of people reported an issue with this hasn't gone up significantly, so I don't see how that would invalidate the previous discussion.

The problem is that it's a long way from "PHPCS hangs on my system and I don't know why" to "hey, there's a broken extension on my computer that fucks things up in a most unexpected way". So, even if people encounter this issue, they might not be reporting it in this thread.

For my current project, it was more like, "Hey you said, we should use PHPCS. So I tried it out, but it didn't work and I gave up."

Seeing it fail in certain environments without any feedback whatsoever pains me.

That's the part where I'm definitely open to suggestions.

I we were to provide that feedback at runtime, we'd need to make the PHPCS codebase aware of gRPC which is the one thing you don't want to happen, isn't it. And respectfully, as long as we can't agree on this point, any suggestion that I could provide would be a waste of everybody's time.

The problem with that, as I said before, is that it opens the door to adding work-arounds for other things which PHPCS doesn't use.

I do see the slippery slope here, no worries. For me the question is: do we want to define the runtime PHPCS has to be run in or do we want PHPCS to run inside the project's runtime? It's been my impression that PHPCS decided in favor of the latter (otherwise, we would not need to support PHP releases that are older than my children). And if it's feasible to ship compat code for PHP 5.4 (which I would really call exotic nowadays), then I believe we can also ship a tiny bit of compat code for…

this exotic extension

---

Side note 1: Thank you for taking your time. The way you've communicated with me last week, by throwing quotes at me without putting them into context, has been a frustrating experience for me, but our exchange today helped me a lot to understand your point of view better.

Side note 2: Maybe we could also consider implementing an alternative parallel execution engine that does not rely on PCNTL. Not just to fix this issue, but also to accomodate users of exotic operating systems like the one from Redmond.