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

PHP gRPC extension breaks use of --parallel #294

Closed
3 tasks done
mbomb007 opened this issue Jan 19, 2024 · 18 comments
Closed
3 tasks done

PHP gRPC extension breaks use of --parallel #294

mbomb007 opened this issue Jan 19, 2024 · 18 comments

Comments

@mbomb007
Copy link

mbomb007 commented Jan 19, 2024

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 = 
  1. 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
Copy link
Member

jrfnl commented Jan 19, 2024

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

@mbomb007
Copy link
Author

I use WSL2 on Windows. Parallel works in WSL.

@mbomb007 mbomb007 changed the title PHP gRPC extensions breaks use of --parallel PHP gRPC extension breaks use of --parallel Jan 19, 2024
@mbomb007
Copy link
Author

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
Copy link
Member

jrfnl commented Jan 19, 2024

@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
Copy link
Author

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
Copy link
Member

jrfnl commented Jan 20, 2024

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

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:

@dingo-d
Copy link
Contributor

dingo-d commented Jan 22, 2024

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
Copy link
Author

mbomb007 commented Jan 22, 2024

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
Copy link

tscni commented Jan 22, 2024

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
Copy link
Author

mbomb007 commented Jan 22, 2024

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
Copy link
Author

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

@jrfnl
Copy link
Member

jrfnl commented Jan 22, 2024

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
Copy link
Author

mbomb007 commented Jan 22, 2024

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
Copy link
Author

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

@mbomb007
Copy link
Author

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

@jrfnl
Copy link
Member

jrfnl commented Jan 23, 2024

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

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
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants