8000 Improve default `StreamSelectLoop` to report any warnings for invalid streams passed to `stream_select()` by clue · Pull Request #245 · reactphp/event-loop · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 22, 2022
Merged < 8000 /span>

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

merged 2 commits into from
Feb 22, 2022

Conversation

clue
Copy link
Member
@clue clue commented Feb 22, 2022 •
edited
Loading

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
@clue clue requested a review from WyriHaximus February 22, 2022 11:43
oleg-andreyev
oleg-andreyev reviewed Feb 22, 2022
View reviewed changes
@WyriHaximus WyriHaximus merged commit 4d3e4aa into reactphp:master Feb 22, 2022
@clue clue deleted the warnings branch February 23, 2022 06:12
@clue clue mentioned this pull request Feb 23, 2022
Merged
@clue clue mentioned this pull request Apr 18, 2022
Merged
@Maikuolan Maikuolan mentioned this pull request Jul 3, 2022
Closed
@clue clue mentioned this pull request Mar 14, 2023
Open
@clue clue mentioned this pull request Apr 13, 2023
Merged
@clue clue mentioned this pull request May 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsor jsor jsor approved these changes

@oleg-andreyev oleg-andreyev oleg-andreyev left review comments

@WyriHaximus WyriHaximus WyriHaximus approved these changes

@SimonFrings SimonFrings SimonFrings approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@clue @jsor @WyriHaximus @oleg-andreyev @SimonFrings
0