Extend pcntl_waitid with rusage parameter #15921
Conversation
vrza
force-pushed
the
waitid-rusage
branch
11 times, most recently
from
729ced4 to
ed5fcbd
Compare
IMHO, I would not worry that much about it. This PR qualifies more for the next major release. Ideally, the existing PR doc should be merged during the 8.4 doc rollercoaster. Merging this PR later and adding relevant infos in the doc. |
vrza
force-pushed
the
waitid-rusage
branch
2 times, most recently
from
9c27dde to
2254c05
Compare
That's fine, as this extension is fully backwards compatible with existing code.
Cool, all comments in that doc PR have been addressed and it's ready for another round of review. |
|
The FreeBSD test pass for me locally otherwise. |
|
They pass for me locally as well. Is there an easy way to see test logs from the Cirrus CI run? |
|
Changed unit test code to synchronize parent and child on every step. |
vrza
force-pushed
the
waitid-rusage
branch
5 times, most recently
from
f515815 to
6166f2c
Compare
|
Found a rare race condition in the unit test, where the child could receive a signal just after signal_dispatch, but just before sleep, in which case the signal would would not interrupt the sleep, and the test would take a long time to complete. Most direct solution I could think of was replacing this code with a quick (starting at 100 ns) exponential backoff loop, so that the race condtion is neutralized, but the unit test should still complete in milliseconds. @devnexen are you able to run that FreeBSD CI workflow again? |
|
Yes the test works for me locally. |
|
@devnexen Okay, great. I also had a colleague test it on macOS, macOS provides (undocumented) POSIX-compatible waitid, but doesn't support I self-reviewed one more time, and it looks good to go, but let's let others take a look as well. //cc @bukka @kocsismate |
|
I will take a proper look later. It's too late for PHP 8.4 so this should wait after branching it out anyway. |
|
Bumping this up for code review since it's been some time. @devnexen @bukka @petk @kocsismate @Girgias |
|
As for me, the code itself looks good. Maybe @kocsismate if possible double check the stub part ? |
If you could rebase/regenerate the stub, would be nice thx. |
|
Rebased, regenerated stub, tests passed. |
| /** @param array $info */ | ||
| function pcntl_waitid(int $idtype = P_ALL, ?int $id = null, &$info = [], int $flags = WEXITED): bool {} | ||
| /** | ||
| * @param array $info |
There was a problem hiding this comment.
not sure if we have to bother with these thus asking the stub wizard assistance :) once confirmed I ll merge shortly. Thanks for your work.
There was a problem hiding this comment.
@kocsismate Could you please take a quick look at the stub?
This functionality is not part of the POSIX interface. - On FreeBSD, the wait6 system call provides it - On Linux, the raw waitid system call provides it (glibc does not) Changed int to pid_t
|
Ah sorry, looks like I completely forgot about this. I just added this to my priority list so I should definitely take a look in the next few weeks. |
| @@ -43,6 +45,9 @@ if test "$PHP_PCNTL" != "no"; then | |||
| ]),,, | |||
| [#include <sys/wait.h>]) | |||
|
|
|||
| AC_CHECK_DECLS([SYS_waitid],,, | |||
There was a problem hiding this comment.
@devnexen This is sort of an off topic but why SYS_pidfd_open is not checked in this way too. There is a check for pidfd_open function but how is that actually passing?
There was a problem hiding this comment.
yes indeed, I'll change that very soon. pidfd_open being a glibc wrapper exclusive (for now at least), it all worked for me locally.
| memcpy(&rusage, &wrusage.wru_self, sizeof(struct rusage)); | ||
| # else /* Linux */ | ||
| memset(&rusage, 0, sizeof(struct rusage)); | ||
| status = syscall(SYS_waitid, (idtype_t) idtype, (id_t) id, &siginfo, (int) options, &rusage); |
There was a problem hiding this comment.
Not sure if I like using undocumented syscalls ... There are already some syscalls used here so I'm not objecting but not sure if there is much value in it.
There was a problem hiding this comment.
@bukka Thanks for the review!
I agree that having to use the raw waitid syscall is not very elegant. However the rusage argument to it is actually documented in the wait(2) man page (under VERSIONS). So Linux officially supports this, it's just that glibc seems to be lagging slightly with the wait6 wrapper.
I think it's nice to round off the PHP pcntl_wait* function family functionality with a rich and consistent API, such as FreeBSD and NetBSD have with wait3, wait4 and wait6 (OpenBSD doesn't yet have wait6).
GNU libc has had wait3 and wait4 for a long time, and historically they were aiming at full support for the BSD APIs as well as POSIX and SysV, but it seems that they haven't gotten around to adding wait6 yet. When glibc gets wait6 we can simplify these ifdefs slightly. If I find time I'll look into sending them a patch.
|
Thanks and sorry for the delay ! |
Enhancement to #14616.
This functionality is not part of the POSIX waitid interface.
wait6system call provides itwaitidsystem call provides it, but it is not part of the interface exposed by glibc.Conceptually, this is similar to how
pcntl_waitandpcntl_waitpidextend the POSIX interface to fetch rusage info, where supported by the host OS.Requesting code review @devnexen @bukka @petk @kocsismate @Girgias
Documentation PR for pcntl_waitid is not merged yet, I can add to that open PR, or document this in a separate one, depending on what PR we merge first.
[edit] Created a documentation PR.