Repository navigation
Conversation
Member
Author
|
@nicolas-grekas Please have a look as you did the original fallback implementation. |
nicolas-grekas
left a comment
nicolas-grekas
left a comment
Contributor
There was a problem hiding this comment.
Note 1 is a bug, note 2 adds a test for it, note 3 is optional. With all three, the suite passes on a fresh PHP-8.6 build and on 8.5 with the polyfill.
Unrelated to this PR, setup-php fails to install uv-amphp/ext-uv@master on every leg, main included. ext-uv has no master branch anymore, only 0.3.x. So UvDriverTest is always skipped on CI, the new skips included.
One case CI can't check: STDIN on a terminal on macOS. If kqueue refuses a tty with EINVAL, the driver now treats the tty as always ready and the loop spins. Worth a quick try on a Mac?
Contributor
|
For the Windows part, php/php-src#25135 makes the WSAPoll backend refuse pipes and files with ERROR_NOSUPPORT, instead of accepting them and never reporting them. Pipes can't be treated as always ready though, so keeping the driver off on Windows makes sense for now. |
epoll and kqueue refuse /dev/null. IoPollDriver moves such streams to a poll() context and treats them as always ready, which forces a zero timeout on every tick. On macOS poll() never reports /dev/null as ready, so the loop busy-waits without ever running the callback. ext-uv refuses /dev/null outright, so UvDriver skips the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
libevent passes on the EPERM of epoll_ctl() for /dev/null instead of falling back, so EventDriver cannot watch it at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IoPollDriver uses kqueue on macOS and WSAPoll on Windows, which behave differently from epoll. The event loop extensions are only installed on Linux, and the style fixer only runs there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…river Streams epoll or kqueue refuse were moved to a poll() context, but treated as always ready regardless, forcing a zero timeout on every tick. On macOS poll() never reports devices such as /dev/null, so the loop busy-waited without ever invoking the callback. The backends only refuse handles that never block, so they are now tracked as always ready without a second context, and their callbacks run on every tick, as stream_select() reports them. Only the errors for handles that cannot be polled (EPERM from epoll, EINVAL from kqueue) take that path, other failures to add a handle are thrown. kqueue accepts regular files, but stops reporting them readable at EOF, so a reader never saw the end, e.g. of STDIN redirected from a file. Regular files are recognized by PHP's seekable flag, which PHP sets when opening the stream, and treated as always ready as well. ext-uv and libevent refuse regular files and /dev/null, so UvDriver and EventDriver skip the tests for them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….0RC4 The kqueue backend of Io\Poll leaks memory on every Watcher::modifyEvents() in PHP 8.6.0RC4, see php/php-src#25086. Only the onReadable and onWritable data sets modify watchers, the others still run. Other versions and backends are tested as usual. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…8.6.0RC4 CI runs a PHP 8.6 development build, so the version check never matched there. The onReadable and onWritable data sets of testNoMemoryLeak are now marked skipped when they fail with kqueue, see php/php-src#25086. They pass again on their own once PHP is fixed, and every other data set and backend still fails as usual. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI runners on macOS oversleep by more than the 0.1 seconds the upper bounds allowed, for every driver. The bounds are now derived from the bug each test guards against: the timer counted from activation after blocking (1.5 seconds instead of 1), and the timer counted from the tick start before blocking (1 second instead of 1.5), which the lower bound already catches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Windows job was cancelled at the 5 minute limit in the middle of the tests. Setting up PHP and installing dependencies takes a large part of that on Windows runners. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WSAPoll, the only Io\Poll backend on Windows, accepts pipes such as STDOUT, but never reports them and returns from wait() immediately, so callbacks on them never run and the loop spins. stream_select() handles pipes, so StreamSelectDriver stays the default there for now. The IoPollDriver tests are skipped on Windows, as they hang on the callbacks on STDOUT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test measures with microtime(), the drivers schedule timers with hrtime(). On Windows the timer fired 0.05 ms before 1.5 seconds by microtime(). The sibling test already allows 0.5 ms for this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventDriver fired 0.56 ms early on Linux, beyond the 0.5 ms tolerance: libevent works with millisecond timeouts and a cached clock. 5 ms covers that and the clock drift on Windows, and is still far from the bugs the tests guard against, which are off by half a second. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- epoll's EPERM is reported as ERROR_NOSUPPORT since php/php-src#23805 (8.6.0RC1), not ERROR_PERMISSION, so streams such as /dev/urandom threw on Linux instead of being treated as always ready. - The spinning test uses /dev/urandom on Linux, where PHP flags /dev/null as seekable, so it never reached epoll and CI missed the wrong error code. - The seekable check only applies with kqueue. On Linux seekable is also true for descriptors epoll can poll, such as eventfd, while epoll refuses regular files by itself. On macOS and BSD, PHP never flags pipes or devices as seekable. Co-Authored-By: Nicolas Grekas <nicolas.grekas@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The macOS and Windows jobs don't install the event loop extensions, and Windows has no sudo to run PIE with. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kelunik
force-pushed
the
io-poll-refused-stream-spin
branch
from
November 1, 2026 21:57
6d24581 to
c577773
Compare
Member
Author
|
@nicolas-grekas Thanks! Addressed the feedback. I'd still like thoughts from @trowski on the fallback behavior, too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two problems found in the review of the IoPollDriver (#128).
Busy loop on streams the backend refuses
epoll and kqueue refuse handles that can't be polled, such as
/dev/null. The driver moved them to a secondpoll()context and forced a zero timeout on every tick, assuming they were always ready. On macOS,poll()doesn't report devices at all, so the loop busy-waited and never invoked the callback.These handles never block, so they're now tracked as always ready, without a second context, and their callbacks run on every tick, as
stream_select()reports them. Only the errors for handles that can't be polled (EPERM from epoll, EINVAL from kqueue) take that path. Any other failure to add a handle, such as running out of epoll watches, is thrown instead of silently degrading.Regular files never reaching EOF on macOS
kqueue accepts regular files, but stops reporting them readable at EOF, so a reader never saw the end, for example of STDIN redirected from a file (
php script.php < input.txt). Regular files are now recognized by PHP'sseekableflag, which is set when the stream is opened, so the check needs no syscall, and treated as always ready too.STDIN from a file, a pipe and
/dev/nullnow behaves the same as withStreamSelectDriver.Tests and CI
testRefusedStreamDoesNotSpinandtestEmptyRegularFileIsReadableare new tests in the baseDriverTest, so they run for every driver. ext-uv and libevent refuse regular files and/dev/nulloutright, soUvDriverTestandEventDriverTestskip them.Io\Pollin PHP 8.6.0RC4 leaks memory on everyWatcher::modifyEvents()(Io\Poll kqueue backend leaks memory on every Watcher::modifyEvents() call php/php-src#25086). TheonReadable/onWritablecases oftestNoMemoryLeakare skipped for that exact version and backend.