Skip to content
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

Improve default StreamSelectLoop to report any warnings for invalid streams passed to stream_select() #245

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

clue
Copy link
Member

@clue clue commented Feb 22, 2022

This changeset improves the default StreamSelectLoop to report any warnings for invalid streams passed to stream_select(). The underlying function would report errors if any streams can not be used to watch for any changes.

This can be trivially reproduced by using a stream with any filters attached:

stream_filter_append(STDIN, 'string.rot13');

Loop::addReadStream(STDIN, function () { });

// PHP Warning:  stream_select(): Cannot cast a filtered stream on this system in X on line Y

Likewise, this will now report a warning if the underlying file descriptor (FD) number exceeds FD_SETSIZE (defaults to 1024 on many platforms). For example, this can be reproduced by running an HTTP benchmark that creates a large number of concurrent connections to a server that allows more than 1024 connections (ulimit may need to be adjusted first) and relies on StreamSelectLoop with the above defaults:

Warning: stream_select(): You MUST recompile PHP with a larger value of FD_SETSIZE.
It is set to 1024, but you have descriptors numbered at least as high as 2799.
 --enable-fd-setsize=3072 is recommended, but you may want to set it
to equal the maximum number of open files supported by your system,
in order to avoid seeing this error again at a later date. in X on line Y

The documentation already contains instructions to use a different loop implementation that does not impose these limitations in this case: https://github.com/reactphp/event-loop#streamselectloop (via #127)

The previous behavior suppressed any warnings and may result in a busy loop that consumes 100% CPU usage by continuously jumping to the next tick. This has improved slightly with PHP 8+ as the stream_select() call would now throw if no valid streams remain in the loop, but similar behavior can still be observed with more (valid) streams attached.

The error suppression operator has originally been introduced via #44 a while back in order to suppress any superfluous warnings that occur when the system call is interrupted by a signal (signal handling support). The same warning still triggers on all PHP versions, so the changeset includes additional checks to ignore only this specific warning and report any other warnings as expected. The test suite confirms this has full test coverage and is supported across all supported PHP versions.

Arguably, the default behavior should be to throw an Exception if any invalid streams are passed to the loop. However, this would incur a significant BC break. I consider reporting a warning instead of simply "hanging" to be a much better solution and believe this to be the best compromise at this point. If you want to throw an Exception in this case, you may register a global error handler that can catch any warnings as expected.

I've also performed a number of benchmarks to check what kind of performance implications this changeset has. It seems the impact for normal operation that has any reasonable stream activity is completely negligible. In a worst-case scenario with a single idle stream and a busy loops constantly using futureTick() for the next tick shows a ~6% performance degradation. I'm going to argue that better error reporting outweighs this small impact. In either case, I've started looking into an unrelated performance improvement that shows a 100% performance improvement for this specific case, but more on that in a separate PR.

I've kept this at two commits to show how ignoring only a specific warning is relatively easy, yet reporting other errors upstream and providing matching test cases is surprisingly non-trivial. You're looking at 8+ hours of work for this PR, enjoy!

@clue clue added this to the v1.3.0 milestone Feb 22, 2022
src/StreamSelectLoop.php Show resolved Hide resolved
src/StreamSelectLoop.php Show resolved Hide resolved
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.

5 participants