Implement pcntl_waitid

[vrza]

Closes #14616

To round out the support for the "wait" family of functions this PR adds the pcntl_waitid function.

Here's the POSIX reference to waitid:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/waitid.html

The waitid() function shall obtain status information pertaining to termination, stop, and/or continue events in one of the caller's child processes

Since status information struct is shared with sigwaitinfo, relevant code from pcntl_sigwaitinfo has been reused.

Note to maintainers: if this patch is accepted, I'll be happy to provide the English documentation in the follow-up PR.

[devnexen]

Note: you can perfectly add non standard flags but you need to make sure they exist before defining them

#ifdef P_PIDFD
/**
 * @var int
 * @cvalue LONG_CONST(P_PIDFD)
 */
const P_PIDFD = UNKNOWN;
#endif

FreeBSD has its own non standard flags if you want to add them too.

Once you fixed all, we need a test for this.

[vrza]

@devnexen Thanks for the quick review! I addressed your comments (ValueErrors, unit test, guard for the Linux speciifc P_PIDFD), please take another look.

[devnexen]

I see the progress but take your time :) I ll have a look later.

[devnexen]

starting to look good, but I ll let it rest enough days to give a chance to other reviewers :).

[vrza]

Please review this patch, it is done and tested to work on:

• GNU/Linux
• OpenBSD
• NetBSD
• FreeBSD
• DragonFly BSD
• macOS [1]
• Illumos

Error handling is done in POSIX style, by accessing errno through pcntl_get_last_error, and (already existing) constants PCNTL_ECHILD, PCNTL_EINTR and PCNTL_EINVAL, that can also be passed to pcntl_strerror to obtain an error message. This is in line with how most other POSIX wrapper functions in PHP are implemented, in particular the implementation is consistent with close relatives pcntl_wait and pcntl_waitpid which I reviewed and found to have satisfactory API design.

[1] On macOS, though, existence of waitid(2) is undocumented, but it seems to work for the POSIX-compliant tested use cases.

[devnexen]

The FreeBSD failing test seems legit.

[devnexen]

@petk what do you think build system wise ?

[vrza]

The FreeBSD failing test seems legit.

@devnexen Right, there was a race condition in the test. Fixed it, thanks for noticing and raising.

On a related note, in multiple other unit tests there are guards that skip them on FreeBSD. I suspect many of them could also be due to incorrect (buggy) test code.

For example, there is an issue in pcntl/tests/002.phpt, that was silenced by Nikita Popov in 2021:

74859783398 (Nikita Popov       2021-06-11 11:57:42 +0200 15) elseif (str_contains(PHP_OS, 'FreeBSD')) die('skip Results in parallel test runner hang on FreeBSD');

I took a quick look at that test code, looks like a race condition (SIGTERM can be sent too early). I could fix that one as well, but as a separate issue/PR.

[petk]

@petk what do you think build system wise ?

Build system changes look very good and professional. Even nicely checked those HAVE_DECL_ ones with #if defined 🤝 Except maybe those two #include arguments can be wraped into quotes without new lines in them: [#include <sys/wait.h>]

[vrza]

@petk Thanks for reviewing 🤝 The aim is to be strict and explicit about requirements, but flexible about the way they are provisioned.

Regarding m4 formatting... Starting with a disclaimer that this is purely a cosmetic discussion with no impact to functionality... Some of those m4 lines are very long, I'd like to break them up somewhere for readability, popping the includes out seemed like a nice place to break the line, although the list of declarations is also rather long. Eventually includes turned out to be a short, unprotected, one-liner.

Perhaps a compromise, with Lisp-style trailing braces:

  AC_CHECK_FUNCS([WIFCONTINUED],,
    [AC_CHECK_DECL([WIFCONTINUED], [AC_DEFINE([HAVE_WIFCONTINUED], [1])],,
      [#include <sys/wait.h>])])

  AC_CHECK_DECLS([WCONTINUED, WEXITED, WSTOPPED, WNOWAIT, P_ALL, P_PIDFD, P_UID, P_JAILID],,,
    [#include <sys/wait.h>])

[petk]

@petk Thanks for reviewing 🤝 The aim is to be strict and explicit about requirements, but flexible about the way they are provisioned.

Regarding m4 formatting... Starting with a disclaimer that this is purely a cosmetic discussion with no impact to functionality... Some of those m4 lines are very long, I'd like to break them up somewhere for readability, popping the includes out seemed like a nice place to break the line, although the list of declarations is also rather long. Eventually includes turned out to be a short, unprotected, one-liner.

Perhaps a compromise, with Lisp-style trailing braces:

  AC_CHECK_FUNCS([WIFCONTINUED],,
    [AC_CHECK_DECL([WIFCONTINUED], [AC_DEFINE([HAVE_WIFCONTINUED], [1])],,
      [#include <sys/wait.h>])])

  AC_CHECK_DECLS([WCONTINUED, WEXITED, WSTOPPED, WNOWAIT, P_ALL, P_PIDFD, P_UID, P_JAILID],,,
    [#include <sys/wait.h>])

Perfect. Yes that's how I was thinking also. :D

[devnexen]

Looking ok by me, cc @kocsismate

[vrza]

While this is in review I started working on documentation.

[bukka]

Looks reasonable

[devnexen]

Since it targets master, we can always improve/fix upon if needs be. I ll merge it in few days if no disagreement arise in the meantime.

[kocsismate]

why is this parameter nullable? Apparently, it's not used anywhere.

[vrza]

It's a minor style point. From the API design standpoint it's not strictly necessary to allow id = null, but since the id parameter will be ignored in the characteristic case of idtype = P_ALL, it can allow PHP programmers to better express their intent and help readability.

For example, compare:

pcntl_waitid(P_ALL, null, $info);

with

pcntl_waitid(P_ALL, 0, $info);

or even

pcntl_waitid(P_ALL, 42, $info);

They all do the same thing, but IMHO the first version best communicates to the person reading the PHP code that we don't really care about the value of the second param (id).

In the C code, the id_is_null value is not used anywhere, as passing 0 to the system call will suffice, but I'm not sure if it's possible to allow a parameter to be null without using this Z_PARAM_LONG_OR_NULL macro.

[kocsismate]

Thanks for the elaborate answer! I support the decision fully, it just looked weird at the first sight that id_is_null was unused, but after thinking about it a little bit, we have plenty of similar parameters in php-src where 0 behaves like null. So I'm sorry about the noise!

[vrza]

No problem, thanks for taking the time to review the code. There's a follow up PR, that adds a non-POSIX rusage parameter on systems that support it, that could also benefit from some additional eyeballs on it.