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