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

PHP8: Uncaught ValueError: No stream arrays wered pased in StreamSelectLoop:: #220

Closed
holtkamp opened this issue Jan 19, 2021 · 8 comments
Closed

Comments

@holtkamp
Copy link

holtkamp commented Jan 19, 2021

It seems this kind of error suppression causes fatal errors on PHP 8

Is there any way to circumvent such error to cause a fatal error?

Also see dpovshed/octopus#45 for a way how to reproduce...

When removing the error suppression by changing

 $ret = @\stream_select($read, $write, $except, $timeout === null ? null : 0, $timeout);

to

 $ret = \stream_select($read, $write, $except, $timeout === null ? null : 0, $timeout);

The following error is generated:

Cannot cast a filtered stream on this system

When changing

if ($read || $write) {

to

if ($read && $write) {

it works, but at this moment I have no idea what other effects this might have... 😕

@clue
Copy link
Member

clue commented Jan 20, 2021

@holtkamp Thanks for reporting, I can see a possible problem and understand where you're coming from!

The following error is generated:

Cannot cast a filtered stream on this system

This is the actual problem you're seeing – plus the existing error handling isn't particularly helpful either.

Streams that have a filter attached MUST NOT be passed to stream_select(). For instance, here's the relevant part of my documentation for clue/stream-filter:

Note that once a filter has been added to stream, the stream can no longer be passed to stream_select() (and family).

Warning: stream_select(): cannot cast a filtered stream on this system in {file} on line {line}

This is due to limitations of PHP's stream filter support, as it can no longer reliably tell when the underlying stream resource is actually ready. As an alternative, consider calling stream_select() on the unfiltered stream and then pass the unfiltered data through the fun() function.

Unfortunately, there doesn't seem to be a way to check whether a stream resource has any filters attached. This means it's up to you to ensure that streams passed to the event loop are "valid".


Additionally, the existing error handling isn't particularly helpful.

If you pass a stream with a filter attached to stream_select(), PHP will automatically emit a warning message for each such stream. Additionally, it will filter out this stream from the list of streams it actually checks.

Warning: stream_select(): Cannot cast a filtered stream on this system in {file} on line {line}

If you do not pass any streams to stream_select(), PHP < 8 will emit a warning message and abort the operation without invoking the underlying select() call. As of PHP 8, this will throw a ValueError instead. This is something we explicitly avoid in our code, because we may still want to check any pending timers even if no streams are being watched.

Warning: stream_select(): No stream arrays were passed in {file} on line {line} (PHP < 8)
Fatal error: Uncaught ValueError: No stream arrays were passed (PHP 8)

Both combined however means that we have no idea that PHP filters out these invalid streams and as such we could end up with an empty array of streams, thus running into the problem again.

On top of this, we do not report any errors generated by stream_select(). This is necessary because the function will also emit a warning message when the select() call is interrupted by a system signal. This is a perfectly valid situation, but unfortunately PHP doesn't appear to expose a reasonable way to know the error reason. The underlying select() call returns a respective errno, but I don't see how this would be exposed on the PHP level.

We may want to look into using error_get_last() to check the actual error message in the future. However, this only exposes the very last error message (and as discussed above, there may be multiple independent errors). As an alternative, we may also set up a temporary error handler, but this will likely incur some noticeable overhead, so I'm not sure this is something we want to have on our hot path.

Sorting this out definitely requires a non-trivial amount of work to check out, but I suppose we're happy to accept PRs. :shipit:

The gist to reproduce this: https://3v4l.org/b8gYu


This combined means you're dealing with an error that is indeed very hard to see. The good news is that this should be relatively easy to fix on your end by making sure you're not passing any streams with a filter attached as discussed above.

I've double checked this and don't see any way this would interfere with PHP 8 other than the above observation. The code base has full code coverage and is tested against PHP 8 and older versions, so I don't see anything that would be broken here at the moment.

This turned out to be long wall of text. I'm curious what others have to add, any input is welcome 👍

@holtkamp
Copy link
Author

holtkamp commented Jan 20, 2021

Thanks for the greatly appreciated wall of text!

By searching for Cannot cast a filtered stream on this system I did find that documentation yesterday.

The good news is that this should be relatively easy to fix on your end by making sure you're not passing any streams with a filter attached as discussed above.

As far as I am concerned the referenced project "octopus" itself does not use any filters. When searching in the vendor folder for any signs of stream filtering, I encountered this:

https://github.com/ringcentral/psr7/blob/360faaec4b563958b673fb52bbe94e37f14bc686/src/InflateStream.php#L24

However, I do not see any indication this InflateStream is actually used.

Local file versus remote file
Note that when loading a local file, the error does not occur.

Together with the observation that using if ($read && $write) { instead of if ($read || $write) { circumvents the problem. Some further debugging indicates that the array with write streams is empty just before the error is triggered.

So what would be the suggested approach to ensure that the array with $write streams is not empty, probably caused by a remote file that has not started downloading yet?

Off-topic
Is there particular reason a fork of https://github.com/guzzle/psr7/blob/master/src/InflateStream.php is used?

@clue
Copy link
Member

clue commented Jan 20, 2021

As far as I am concerned the referenced project "octopus" itself does not use any filters.

We're not aware of any such issues in any other projects, so I wonder what could be the cause. I'm not familiar with the octopus code base, but perhaps you can provide a simple gist to reproduce this? 👍

Local file versus remote file
Note that when loading a local file, the error does not occur.

Can you be more specific what "local file" means here? A simple gist would help 👍

Together with the observation that using if ($read && $write) { instead of if ($read || $write) { circumvents the problem. Some further debugging indicates that the array with write streams is empty just before the error is triggered.

Unfortunately, this will break existing logic and test cases. The code is correct as-is. If you change this to &&, it means you're no longer watching readable streams when no writable streams exist (and vice versa).

Off-topic
Is there particular reason a fork of https://github.com/guzzle/psr7/blob/master/src/InflateStream.php is used?

See reactphp/http#331

@holtkamp
Copy link
Author

holtkamp commented Jan 20, 2021

You can have a look at the issue I mentioned dpovshed/octopus#45 for an example on how to reproduce, this also indicates what I mean with "local" file 😉

It comes down to run the following on a PHP 8 environment:

git clone https://github.com/dpovshed/octopus
cd octopus
composer install
php application.php https://www.rocktherankings.com/sitemap.xml

@clue
Copy link
Member

clue commented Jan 20, 2021

This is a bug in octopus:

The gist of what's happening here:

$f = fopen('https://www.rocktherankings.com/sitemap.xml', 'r');
$loop->addReadStream($f, function () { });

The fopen() call in there is blocking and must not be used in a non-blocking application. It looks like this will automatically assign a stream filter to deal with HTTP compression.

In order to send non-blocking HTTP requests, https://github.com/reactphp/http should be used instead. It looks like octopus already uses similar logic to send HTTP requests for each URL it finds in the sitemap. It needs to use the same logic to download the sitemap itself.

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍

@clue clue closed this as completed Jan 20, 2021
@kelunik
Copy link
Contributor

kelunik commented Jan 20, 2021

@clue See amphp/amp@73f6e71 for a possible implementation for avoiding the suppression. Feel free to copy with attribution.

@holtkamp
Copy link
Author

Big thanks for #220 (comment)

Indeed the initial step of the process of the example is blocking. This problem was "suppressed" when used on PHP < 8 and now surfaced.

@clue
Copy link
Member

clue commented Feb 18, 2021

@holtkamp Glad to hear this has been tracked down successfully!

For future reference, here's the relevant section from the PHP documentation for "Backward Incompatible Changes" in PHP 8:

The HTTP stream wrapper as used by functions like file_get_contents() now advertises HTTP/1.1 rather than HTTP/1.0 by default. This does not change the behavior of the client, but may cause servers to respond differently. To retain the old behavior, set the 'protocol_version' stream context option, e.g.

<?php
$ctx = stream_context_create(['http' => ['protocol_version' => '1.0']]);
echo file_get_contents('http://example.org', false, $ctx);
?>

https://www.php.net/manual/en/migration80.incompatible.php#migration80.incompatible.standard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants