PHP gRPC extension breaks use of `--parallel`

[mbomb007]

Describe the bug

A clear and concise description of what the bug is.

See https://github.com/grpc/grpc/issues/20250

To reproduce

Steps to reproduce the behavior:

1. Add the grpc PHP extension

2. 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: https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/10

Parent issue: https://github.com/wodby/drupal-php/issues/102

Please confirm:

• [x] I have searched the issue list and am not opening a duplicate issue.
• [x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• [x] 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:

• https://github.com/grpc/grpc/issues/20250
• https://github.com/grpc/grpc/issues/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:

• https://github.com/grpc/grpc/issues/13412
• https://github.com/grpc/grpc/issues/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 https://github.com/grpc/grpc/issues/20250#issuecomment-532319510

[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:

• https://github.com/squizlabs/PHP_CodeSniffer/issues/2767 - this is basically your issue as the OP was also using the Drupal Docker container.
• https://github.com/squizlabs/PHP_CodeSniffer/issues/2983
• https://github.com/squizlabs/PHP_CodeSniffer/issues/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: https://github.com/squizlabs/PHP_CodeSniffer/issues/2767