Reading GzipDecodeStream causes Uncaught RuntimeException: Unable to perform operation on closed stream

[iainconnor]

Q	A
Bug?	yes
New Feature?	no
Version	1.0.1

Actual Behavior

What is the actual behavior?

PHP Fatal error: Uncaught RuntimeException: Unable to perform operation on closed stream in /Users/iconnor/Documents/PHP/php-http-message-bug/vendor/clue/stream-filter/src/functions.php:98

Expected Behavior

What is the behavior you expect?

The stream is rewindable. I should be able to read its contents, rewind it, and then read its contents again.

Steps to Reproduce

Checkout this repository https://github.com/iainconnor/php-http-message-bug/blob/master/demo.php run demo.php.

Or,

$streamFactory = new \Http\Message\StreamFactory\GuzzleStreamFactory();
$stream = $streamFactory->createStream(file_get_contents('http://httpbin.org/gzip'));
$gzipStream = new \Http\Message\Encoding\GzipDecodeStream($stream);

echo $gzipStream->getContents() . PHP_EOL;

if ( $gzipStream->isSeekable() ) {
    $gzipStream->rewind();
    echo $gzipStream->getContents();
}

Possible Solutions

If you have already ideas how to solve the issue, add them here.
(remove this section if not needed)

These lines -- https://github.com/php-http/message/blob/master/src/Encoding/FilteredStream.php#L116-L118 -- look to be the cause, though I'm not sure on the solution.

[joelwurtz]

A gzip stream should not be seekable / rewindable, so i suspect it miss an exception here.

You can use a buffered stream to do that :

$gzipStream = new \Http\Message\Stream\BufferedStream(new \Http\Message\Encoding\GzipDecodeStream($stream));

[iainconnor]

Thanks for the quick reply. That indeed does resolve this behaviour.
Do you think the DecodePlugin should be updated to wrap in a BufferedStream by default?

[joelwurtz]

No it should not, as buffering the stream is not so useful (only when you need to read multiple times the body) and consume memory / io and will slow down applications when dealing with big body.

[iainconnor]

Thanks, I think that makes sense.
Though, I feel this behaviour should go into a document somewhere.

If you build a client like this...

return new PluginClient(
  $httpClient,
  [
    CachePlugin::clientCache($cache, $streamFactory),
    new DecoderPlugin([])
  ]
)

... which I think is reasonable, because you want your cache to be as efficient as possible, so putting it at the top of the chain makes it so it has the opportunity to satisfy the request from cache before doing unnecessary operations, then you'll run into this crash because CachePlugin will do...

$bodyStream = $response->getBody();
$body = $bodyStream->__toString();
if ($bodyStream->isSeekable()) {
  $bodyStream->rewind();
} else {
  $response = $response->withBody($this->streamFactory->createStream($body));
}

... and then, at some point, your application code will probably do a second $response->getBody()->getContents();, resulting in a crash.

Would the recommended behaviour be DecodePlugining first in the chain? Or should GzipDecodeStream return false for ->isSeekable()?

[joelwurtz]

Filtered stream should return false in fact, fixed in #100