Please note that it has got some dependencies to just introduced Polling API Additions RFC and it also has follow up Ring API support that will be introduced if this get accepted. It’s in early stage and there are things to change but the API is in the state that it can be discussed. There is also a working proof of concept that integrates all parts and can be found at https://github.com/php/php-src/pull/23997 .
Please note that it has got some dependencies to just introduced Polling API Additions RFC and it also has follow up Ring API support that will be introduced if this get accepted. It's in early stage and there are things to change but the API is in the state that it can be discussed. There is also a working proof of concept that integrates all parts and can be found at IO hooks PoC by bukka · Pull Request #23997 · php/php-src · GitHub .
If I understand it correctly, then proc_close() and pcntl_waitpid() as WaitPid operations, and pipe reads that suspend instead of block, orchestrating several child processes from PHPUnit's main process may become a lot more convenient.
Looking forward to the discussion (and to the future where I can depend on this in PHPUnit).
My concern is the safety model. With a provider installed every hooked
call becomes a suspension point inside C code written assuming no PHP
runs during IO. The branch guards the stream, curl, Socket,
XMLReader/XMLWriter and three mysqli entry points. With only the Poll
queue I hit or traced:
DOMDocument::save() to a pipe, FIFO or ftp:// target: loadXML() or
removeChild() from another Fiber frees the tree mid-serialize
(valgrind: reads of the doc freed by loadXML; removeChild segfaults).
hash_update_stream(): hash_final() from another Fiber NULLs
hash->context under the read loop (segfault).
libxml_set_streams_context() during a suspended http read frees the
context the redirect then reuses.
mysqlnd shared across Fibers: an unbuffered result can be freed under
fetch_row, a persistent PDO liveness check can free the connection,
and pdo_mysql has no guard at all.
With Files, an include suspends inside opcache’s compile window, so
classes declared meanwhile end up in the cached script.
Guarding objects one at a time won’t converge, and third-party
extensions never opted in. I’d flip the default: hooked calls run
synchronously unless the call site declares itself suspension safe.
From running the branch:
Pipes are not syscall first, so a 10-byte top-level fwrite(STDOUT)
to a pipe, or sleep(), under the RFC’s Scheduler throws FiberError.
The main flow needs a defined rule.
Under a provider STDIN/STDOUT get O_NONBLOCK on the shared file
description; other processes and system() children see it, and it
survives kill -9. Without one, a proc_open child given a socket as
stdin gets EAGAIN from dd. Neither is in the BC section.
An exception unwinding past a registered stream skips remove(), and
the next socket on that fd number is never re-armed.
Two Fibers flock()ing one file deadlock the process.
A provider declaring DirectData can complete Write/Send as Done
without doing anything. I’d keep Files/DirectData/DirectAccept out of
the userland API until the Ring RFC passes.
Could the hooks be split out of the branch into their own PR? At 23k
lines with the Ring and the Poll additions, it’s hard to review.
My concern is the safety model. With a provider installed every hooked
call becomes a suspension point inside C code written assuming no PHP
runs during IO. The branch guards the stream, curl, Socket,
XMLReader/XMLWriter and three mysqli entry points. With only the Poll
queue I hit or traced:
DOMDocument::save() to a pipe, FIFO or ftp:// target: loadXML() or
removeChild() from another Fiber frees the tree mid-serialize
(valgrind: reads of the doc freed by loadXML; removeChild segfaults).
hash_update_stream(): hash_final() from another Fiber NULLs
hash->context under the read loop (segfault).
libxml_set_streams_context() during a suspended http read frees the
context the redirect then reuses.
mysqlnd shared across Fibers: an unbuffered result can be freed under
fetch_row, a persistent PDO liveness check can free the connection,
and pdo_mysql has no guard at all.
With Files, an include suspends inside opcache’s compile window, so
classes declared meanwhile end up in the cached script.
Guarding objects one at a time won’t converge, and third-party
extensions never opted in. I’d flip the default: hooked calls run
synchronously unless the call site declares itself suspension safe.
From running the branch:
Pipes are not syscall first, so a 10-byte top-level fwrite(STDOUT)
to a pipe, or sleep(), under the RFC’s Scheduler throws FiberError.
The main flow needs a defined rule.
Under a provider STDIN/STDOUT get O_NONBLOCK on the shared file
description; other processes and system() children see it, and it
survives kill -9. Without one, a proc_open child given a socket as
stdin gets EAGAIN from dd. Neither is in the BC section.
An exception unwinding past a registered stream skips remove(), and
the next socket on that fd number is never re-armed.
Two Fibers flock()ing one file deadlock the process.
A provider declaring DirectData can complete Write/Send as Done
without doing anything. I’d keep Files/DirectData/DirectAccept out of
the userland API until the Ring RFC passes.
Could the hooks be split out of the branch into their own PR? At 23k
lines with the Ring and the Poll additions, it’s hard to review.
It’s actually noted in the PR that it’s still PoC and it’s not supposed to be reviewed yet. But thanks for checking anyway as it’s useful but don’t expect to be in the ready state. I don’t want to split it at this stage as it would be pain to maintain but I will do so closer to the vote (which for hooks quite far). I got actually quite a few of those place on my to do already but there are some new finding so will update it. All of this will be, of course, sorted out. I don’t see this as a design issue as the freezing is actually quite strict and prevents many issues so we should be able to make this safe. In the future we might consider relaxing some of the rules.
For now I’m mainly seeking the API design review and just want to make sure that things can work out in the current implementation.