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

Error when trying to read 0 byte from StreamingBody #1309

Closed
youhyunkim opened this issue Nov 8, 2017 · 1 comment · Fixed by #1312
Closed

Error when trying to read 0 byte from StreamingBody #1309

youhyunkim opened this issue Nov 8, 2017 · 1 comment · Fixed by #1312
Labels
enhancement This issue requests an improvement to a current feature.

Comments

@youhyunkim
Copy link

Referring to the read method of StreamingBody:

def read(self, amt=None):
"""Read at most amt bytes from the stream.
If the amt argument is omitted, read all data.
"""
chunk = self._raw_stream.read(amt)
self._amount_read += len(chunk)
if not chunk or amt is None:
# If the server sends empty contents or
# we ask to read all of the contents, then we know
# we need to verify the content length.
self._verify_content_length()
return chunk

If anyone asks for 0 bytes from a StreamingBody, the conditional on line 76 will pass because chunk is empty (since 0 bytes were asked for) and amount was set to 0 (not None). This leads to the content length verification, which will fail because you've read 0 bytes so far out of the entire content.

Might be an odd use case, but I feel like is a valid use case.
In fact, I ran into this issue when trying to use the ijson package link.
That library uses .read(0) in order to figure out what type of encoding the stream reader should use. Whether that's the best way to do it or not, I'm not entirely sure. But I feel like .read(0) should still be supported.

If you guys agree that it should be supported, maybe considering a condition like this:

if (not chunk and amt > 0) or amt is None:
@joguSD joguSD added the enhancement This issue requests an improvement to a current feature. label Nov 8, 2017
@joguSD
Copy link
Contributor

joguSD commented Nov 8, 2017

Thanks for the PR @youhyunkim. I've cleaned up the PR to pass all unit tests and added a test to validate this change in #1312.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue requests an improvement to a current feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants