Skip to content

Fix GH-10239: proc_close after proc_get_status always returns -1 #10250

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

Closed
wants to merge 5 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jan 6, 2023

Fixes GH-10239

The waitpid function only works once when a process is exited.
Cache the result so subsequent status reads succeed.

@nielsdos nielsdos changed the base branch from PHP-8.1 to master January 7, 2023 14:12
@thg2k
Copy link
Contributor

thg2k commented Jan 7, 2023

Does this also change the behavior of proc_get_status?

@nielsdos
Copy link
Member Author

nielsdos commented Jan 7, 2023

Does this also change the behavior of proc_get_status?

Previously, multiple proc_get_status on the same exited process wouldn't work: only the first one would succeed. Now you can keep calling proc_get_status as many times as you want and it will return the correct status.

@thg2k
Copy link
Contributor

thg2k commented Jan 8, 2023

Does this also change the behavior of proc_get_status?

Previously, multiple proc_get_status on the same exited process wouldn't work: only the first one would succeed. Now you can keep calling proc_get_status as many times as you want and it will return the correct status.

Although I believe this is an improvement, it is technically a breaking change and needs to be documented in the upgrading notes. The behavior is well known and documented in the proc_get_status page (but not in the proc_close page, even though it was also known behavior)

@nielsdos
Copy link
Member Author

nielsdos commented Jan 8, 2023

Although I believe this is an improvement, it is technically a breaking change and needs to be documented in the upgrading notes. The behavior is well known and documented in the proc_get_status page (but not in the proc_close page, even though it was also known behavior)

I at first didn't see the note in the docs. The note is even incomplete as not only the exit code, but also the other values will not be set correctly on the second call.
This PR will only be applied on 8.3 as it targets master atm. I'll add an UPGRADING notice for this soon-ish.

@nielsdos
Copy link
Member Author

Rebased on master, and added a NOTE to UPGRADING.

@bukka
Copy link
Member

bukka commented Jan 13, 2023

I'm not really sure if we want to do this as currently proc_get_status mimic waitpid and might be useful in that way so the users expectations might be the same as for using waitpid. Also it might be a bit confusing as it gives info about process that has no state in kernel. Maybe improving docs would be a better option here. Alternatively we could also consider adding a new parameter to the function to store the info.

@nielsdos
Copy link
Member Author

I'm not really sure if we want to do this as currently proc_get_status mimic waitpid and might be useful in that way so the users expectations might be the same as for using waitpid. Also it might be a bit confusing as it gives info about process that has no state in kernel. Maybe improving docs would be a better option here. Alternatively we could also consider adding a new parameter to the function to store the info.

I understand what you're saying, but I think this reasoning only holds for posix systems. These functions are also supported on Windows, and as far as my understanding goes, the child process handle on Windows is only closed upon calling proc_close. Therefore you can repeatedly read the status on Windows even after the child has exited, because the handle is still open. So the behaviour prior to this PR is different on Windows vs posix systems, and this PR unifies the behaviour. So I'm not so sure people actually expect it to behave like waitpid.

The original bug report was about using proc_close after proc_get_status. This PR fixes that and also changes the behaviour of calling proc_get_status on an exited process multiple times. If we would keep the current behaviour of calling proc_get_status multiple times on an exited process, do we also keep the current behaviour as described in the bug report, or do we still want the fix for that?

@bukka
Copy link
Member

bukka commented Jan 13, 2023

Good point about Windows even though it's a minority OS for PHP. I'm thinking that it might be actually enough to add a field to the result specifying that the result is cached. Then it could still be used as waitpid after some modifications (checking the field instead). I guess this use case of waitpid might not be really something that's used much so if properly added to UPGRADING what needs to be done to keep the same behavior, then it should be fine.

nielsdos and others added 4 commits February 18, 2023 11:56
The waitpid function only works once when a process is exited.
Cache the result so subsequent status reads succeed.
Co-authored-by: pezcurrel <33592962+pezcurrel@users.noreply.github.com>
…atus()

Users who previously would've relied on proc_get_status() failing after
calling it more than one time can now check the "cached" key to see
whether the result was cached. If it is cached, it would've previously
returned a failure in the form of a -1 exit code and the status booleans
all being false.
@nielsdos
Copy link
Member Author

I've updated this with your suggestion. I've added a "cached" key to the array which indicates whether the result was cached.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @nielsdos! Small nit: We're using the two terms stored and cached, can we pick one? @bukka Are you happy with this?

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nielsdos
Copy link
Member Author

Thank you both, I fixed the terminology to always use cached instead of stored.

@iluuu1994 iluuu1994 closed this in 25d6c93 Feb 22, 2023
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Feb 9, 2024
…for PHP Versions Below 8.3 (Luc45)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | N/A <!-- No existing issue, direct PR -->
| License       | MIT

This PR addresses a bug in Symfony's Process component affecting PHP versions prior to 8.3. In these versions, calling `proc_get_status` multiple times on the same process resource only returns the correct exit status on the first call, with subsequent calls returning -1 due to the process being discarded. This behavior can lead to race conditions and incorrect process status reporting in Symfony applications.

PHP 8.3 [changelog notes](https://www.php.net/manual/en/migration83.incompatible.php)/[fix PR](php/php-src#10250):

> Executing proc_get_status() multiple times
>
> Executing proc_get_status() multiple times will now always return the right value on POSIX systems.
> Previously, only the first call of the function returned the right value. Executing proc_close()
> after proc_get_status() will now also return the right exit code. Previously this would return -1.

Symfony Process is very good at avoiding this, but it's possible to trigger this behavior if you pass the Process as a reference to the output callback itself, and inside of it you invoke a method that eventually triggers `proc_get_status`, such as `isRunning`, `isSuccessful`, etc.

This PR tries to mimick PHP 8.3 behavior in older versions, caching the first reported valid exit status code once the process exits, and reusing the cached value if the new exit status code is now invalid.

I believe this bug has been pestering Symfony Process for a long time, as per [this Stack Overflow question](https://stackoverflow.com/questions/31828700/symfony-process-component-returns-exit-code-1/77951961#77951961), but it was very hard to track it down, and only happens in certain conditions, and it also have a racing condition factor depending on the time between the last output of the process and it's termination.

### Changes:
- Added caching mechanism for the exit status in the Process component.
- Ensured the cached status is used for subsequent `proc_get_status` calls if PHP version is below 8.3.

### Proof of Concept:

To demonstrate the issue and the effectiveness of the fix, a proof of concept script is included below. This script uses Symfony's Process component to start a subprocess that outputs a message and exits with a specific status code. The script then attempts to retrieve the exit status of the process using getExitCode(). In PHP versions prior to 8.3, without the proposed fix, this script will often incorrectly report an exit code of -1 due to the race condition described earlier.

```php
<?php

if ( getenv( 'OUTPUT' ) ) {
	#echo 'This should fail when "wait" gets large';
	sleep( 2 );
	echo 'This should fail even when "wait" is small';
	die( 123 );
}

use Symfony\Component\Process\Process;

require_once __DIR__ . '/vendor/autoload.php';

$wait = 0;

do {
	// Increasingly bigger waits.
	$wait += 100000;
	echo sprintf( 'Wait: %s', $wait ) . PHP_EOL;

	$p = new Process( [ 'php', __FILE__ ], __DIR__, [
		'OUTPUT' => 1,
	] );

	$p->start( function ( string $type, string $out ) use ( $p ) {
		echo $out . PHP_EOL;

		/**
		 * Calling most methods in Symfony Process that triggers
		 * updateStatus() can potentially trigger the -1 bug.
		 *
		 * `@see` Process::updateStatus()
		 */
		echo sprintf( 'Is Running: %s', $p->isRunning() ? 'Yes' : 'No' ) . PHP_EOL;
		echo sprintf( 'Exit Code: %s', $p->getExitCode() ) . PHP_EOL;
	} );

	while ( $p->isRunning() ) {
		usleep( $wait );
	}

	if ( ! $p->isSuccessful() ) {
		break;
	}
} while ( true );

$is_started = $p->isStarted();
$is_running = $p->isRunning();
$exit_code  = $p->getExitCode();

echo sprintf( 'Started: %s, Running: %s, Exit code: %s', $is_started, $is_running, $exit_code ) . PHP_EOL;
```

### Impact:
- This change only affects Symfony applications running on PHP versions below 8.3.
- It improves the reliability of process status reporting in these environments.
- No breaking changes or backward compatibility issues are introduced.

### Testing:
I haven't added tests to this PR because:

- This bug involves racing conditions, which is hard to replicate.
- It's past 1 AM local time right now, and I've been debugging this for the past 6 hours.
- The provided "Proof of Concept" can serve as an initial check to confirm the bug.

I'm really not the type of developer that does not write tests, but I beg my pardon for the reasons above.

### Considerations
- Based on the [proc_open](https://github.com/php/php-src/blob/php-8.3.2/ext/standard/proc_open.c) implementation, this fix might not be needed on Windows.
- You can find additional context for this bug in this [Stack Overflow answer](https://stackoverflow.com/a/77951961/2056484).

Commits
-------

500caa3 [Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proc_close after proc_get_status always returns -1
5 participants