Only read up to Content-Length in stream wrapper #1510
Conversation
|
Note that the test failure is due to an unrelated change in a package Guzzle depends on. @Tobion is addressing this issue. |
| $this->drain( | ||
| $stream, | ||
| $sink, | ||
| $response->getHeaderLine("Content-Length") |
There was a problem hiding this comment.
I think single quotes are used so far
There was a problem hiding this comment.
Maybe something like is_numeric($response->getHeaderLine('Content-Length') ? (int) $response->getHeaderLine('Content-Length') : -1 then you can just pass this length to copy_to_stream
mtdowling
force-pushed
the
stream-read-content-length
branch
from
8ff4b3c
to
baef4ac
Compare
|
Good feedback, @Tobion. I've pushed up a commit to address the feedback. |
| Psr7\copy_to_stream( | ||
| $source, | ||
| $sink, | ||
| strlen($contentLength) > 0 ? (int) $contentLength : -1 |
There was a problem hiding this comment.
If Content-Length is not numeric (which would be invalid), it will cast to 0. Not sure if that is wanted.
There was a problem hiding this comment.
Actually, that's probably ideal. It would prevent us from being able to read any data, which is better than reading until a timeout.
There was a problem hiding this comment.
An actual contentLength of 0, or one that is cast to 0 seems to be causing issues with fread in the Psr7 package, see issue #1539.
I hit this fread warning today when calling some AWS API commands which caused my Laravel app to bomb out.
|
Responses to HEAD requests can sent a Content-Length header that is not actually the size of the response body:
Is it handled somewhere that certain status codes or HEAD responses must not have a body? |
mtdowling
force-pushed
the
stream-read-content-length
branch
from
baef4ac
to
1fa6184
Compare
|
Looking at the implementation of copy_to_stream ( https://github.com/guzzle/psr7/blob/master/src/functions.php#L385 ) it seems that it will require the amount of memory that is given with the maxLength size. So if the response is 1 GB, it will also require 1 GB of memory to copy the stream. This doesn't seem right. Shouldn't it read the stream in batches just as it is done when |
|
Yeah, this is not taking HEAD requests into account. I'll push a fix. As for copy_to_stream, you're right. The implementation could be improved. |
This commit updates the stream wrapper to only read up to the number of bytes returned in the Content-Length header when draining a stream synchronously.
mtdowling
force-pushed
the
stream-read-content-length
branch
from
1af7d30
to
a243357
Compare
|
No longer draining the stream for HEAD requests. The poor maxLen implementation can be addressed separately on the PSR-7 repo. Rebased on master, so hopefully the tests will pass now. |
|
|
|
Nice catch. I pushed a commit to no longer create a new sink when sending a HEAD request. |
| $this->drain($stream, $sink); | ||
| // Do not drain when the request is a HEAD request because they have | ||
| // no body. | ||
| if ($hasResponseBody && $sink !== $stream) { |
There was a problem hiding this comment.
$hasResponseBody seems useless as the second condition is never true in this case anyway
mtdowling
force-pushed
the
stream-read-content-length
branch
from
072383b
to
5b3279a
Compare
|
👍 |
…nt-length envoyé par le serveur pour arrêter la lecture du stream des qu'on a atteint cette longueur et eviter de timeout en demandant plus de contenu sur un serveur mal configuré Cf guzzle/guzzle#1510 et guzzle/guzzle#1577 Refs: #5769
…nt-length envoyé par le serveur pour arrêter la lecture du stream des qu'on a atteint cette longueur et eviter de timeout en demandant plus de contenu sur un serveur mal configuré Cf guzzle/guzzle#1510 et guzzle/guzzle#1577 Refs: #5769
…nt-length envoyé par le serveur pour arrêter la lecture du stream des qu'on a atteint cette longueur et eviter de timeout en demandant plus de contenu sur un serveur mal configuré Cf guzzle/guzzle#1510 et guzzle/guzzle#1577 Refs: #5769
This commit updates the stream wrapper to only read up to the number of
bytes returned in the Content-Length header when draining a stream
synchronously.
@Yanacek @Tobion