-
-
Notifications
You must be signed in to change notification settings - Fork 77
--parallel
option fails if pcntl_waitpid returns non-managed process ID
#1112
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
Comments
Thanks for reporting this issue, @NanoSector! I was unable to reproduce it on an Ubuntu 24.04 installation running PHP 8.3. I tested using the following commands:
Both PHPCS and PHPCBF worked as expected. I was also able to confirm using Xdebug that As a side note, the example you provided ( |
Thanks @rodrigoprimo, I'm not really sure what is causing this either, I hadn't looked into it that far. My assumption is that a PHP extension is starting a thread (maybe XDebug?) for some reason as there's no code preceding the phpcbf/phpcs execution. The Ubuntu 22.04 machine in question is a slightly modified Laravel Homestead VM, but I understand that's a bit much to ask to setup to try to reproduce this issue. I'll try to reproduce it on my personal MacBook later tonight, if I can't reproduce it there I'll make a dump of loaded php modules and PHP's ini values and compare the two, in that case I'll get back to you tomorrow. Funny how this uncovered another issue 😅 I was fairly positive that I changed the file names appropriately. |
@NanoSector I look forward to seeing the more detailed environment info tomorrow. I kind of doubt it will be related to Xdebug, as Xdebug is used a lot and I've not seen this issue reported before, but definitely curious to see what is causing it. |
I dug a little deeper, turns out it is not a PHP environment issue but a side effect of a helper command I use to talk to the Vagrant VM from the host, written in Fish. It eventually executes the following command:
Running the following in the VM itself does not exhibit this behaviour:
And moving the
What I think is happening is that Bash is somehow putting the file redirection into the same process group as phpcbf and this trips up the Runner. The in-VM implementation does file redirection before invoking bash with phpcbf. This bug can be triggered from Bash without SSH by the following command:
I do feel like phpcs/phpcbf should at least check that it's monitoring its own processes / ignore foreign processes, regardless of this unusual use case. |
@NanoSector Interesting case for sure. Would there be any way this can be triggered without your custom Vagrant VM set up though ? |
@jrfnl Yes, you should be able to trigger it locally using the last command I provided (replace the path with something pointing to a project with phpcs in it). This works on my MacBook without any VMs involved. |
…rocess The --parallel option spawns processes with pcntl_fork and expects the result of pcntl_waitpid to be one of its spawned processes. It looks up the temporary output file of a process in the given $childProcs array without checking if the returned process ID is actually managed, which under some circumstances caused by external factors may result in an undefined array index error. Fixes PHPCSStandards#1112
…rocess The --parallel option spawns processes with pcntl_fork and expects the result of pcntl_waitpid to be one of its spawned processes. It looks up the temporary output file of a process in the given $childProcs array without checking if the returned process ID is actually managed, which under some circumstances caused by external factors may result in an undefined array index error. Fixes PHPCSStandards#1112
…rocess The --parallel option spawns processes with pcntl_fork and expects the result of pcntl_waitpid to be one of its spawned processes. It looks up the temporary output file of a process in the given $childProcs array without checking if the returned process ID is actually managed, which under some circumstances caused by external factors may result in an undefined array index error. Fixes PHPCSStandards#1112
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts. This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
Uh oh!
There was an error while loading. Please reload this page.
Describe the bug
The returned PID of
pcntl_waitpid
is not checked against the list of known PIDs insrc/Runner.php
. In our setup, this call always returns a PID with a value 1 lower than the lowest expected PID in the $childProcs list which causesphpcs
andphpcbf
to fail.E.g. the $childProcs list contains
[5125, 5126]
, but the function first returns5124
and this causes the runner to crash with the following exception:Code sample
N/A, this bug happens in every project regardless of code snippets.
To reproduce
Steps to reproduce the behavior:
bash -c 'bash --init-file <(echo "cd /home/vagrant/projects/my-project") -i -t -c "vendor/bin/phpcbf --parallel=8 -p test.php test2.php"'
Expected behavior
The parallel option works & phpcs only takes care of managed processes, even when invoked with constructs like these.
Versions (please complete the following information)
Additional context
pcntl_waitpid
already returns the lower value before the first call in pcntl_fork in Runner.php so this is very likely an external factor.Patching Runner.php with the following code is sufficient to fix the issue, but I am not sure if this is the wanted solution:
change to
Please confirm
master
branch of PHP_CodeSniffer.The text was updated successfully, but these errors were encountered: