Decoding Exhausted Stream

[bryanhelmig]

Added a "reference" read() test, as well as a demonstration of how it differs from the streaming behavior for deflate or gzip. "reference" may be too strong of word in the context of urllib3, but seems to hold true for the standard library:

>>> from io import BytesIO
>>> f = BytesIO('foo')
>>> f.read(3)
'foo'
>>> f.read()
''
>>> f.read()
''

As of this PR opening - I expect 3 existing tests to fail and 1 new test to succeed. No solution yet!

[bryanhelmig]

Very interesting that pypy succeeds but cpython fails (as expected!).

[bryanhelmig]

Neat. @shazow ready for merge if you deem it so.

[bryanhelmig]

Side note: I am guessing pypy's implementation of zlib.decompressobj().decompress() handles empty b'' without exception vs cpython's native. Interesting.

[sigmavirus24]

👍

[Lukasa]

👍

[bryanhelmig]

Before anyone merges, is this right? Maybe it needs to flush when data = b'' too.

[sigmavirus24]

I'm honestly not sure what the answer is. @Lukasa ?

[bryanhelmig]

When another data = b'' the following self._decoder.decompress(binary_type()) would fail - if there was a bool(self._decoder.closed) or equivalent - I imagine that would be cleaner than bool(data). 😕

[sigmavirus24]

I think I understand your concern here better now. Basically, "What if we have data that's been decoded that hasn't been flushed yet?"

I think that's a valid usecase and that your problems were mostly arising from the block above, right? If you remove this check, do your tests fail?

[bryanhelmig]

Indeed - if we remove either of those checks some test cases fail. I can throw in some tracebacks of each if that would help.

[sigmavirus24]

I only care about removing this one though. Can you include the tracebacks for the tests that fail when you remove only this one (for the flush)

[bryanhelmig]

The three tests fail when removing the second data check:

======================================================================
ERROR: test_chunked_decoding_deflate (test.test_response.TestResponse)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bryanhelmig/Code/urllib3/test/test_response.py", line 115, in test_chunked_decoding_deflate
    self.assertEqual(r.read(), b'')
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 227, in read
    buf = self._decoder.decompress(binary_type())
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 34, in decompress
    return self.decompress(self._data)
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 25, in decompress
    return self._obj.decompress(data)
error: Error -3 while decompressing: invalid stored block lengths

======================================================================
ERROR: test_chunked_decoding_deflate2 (test.test_response.TestResponse)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bryanhelmig/Code/urllib3/test/test_response.py", line 132, in test_chunked_decoding_deflate2
    self.assertEqual(r.read(), b'')
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 227, in read
    buf = self._decoder.decompress(binary_type())
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 25, in decompress
    return self._obj.decompress(data)
error: Error -2 while decompressing: inconsistent stream state

======================================================================
ERROR: test_chunked_decoding_gzip (test.test_response.TestResponse)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bryanhelmig/Code/urllib3/test/test_response.py", line 149, in test_chunked_decoding_gzip
    self.assertEqual(r.read(), b'')
  File "/Users/bryanhelmig/Code/urllib3/urllib3/response.py", line 227, in read
    buf = self._decoder.decompress(binary_type())
error: Error -2 while decompressing: inconsistent stream state

[bryanhelmig]

It makes sense, binary_type() is an empty string by definition - something _decoder.decompress can't handle (after exhaustion). I wonder if we should just move the check into self._obj.decompress(data)...

[bryanhelmig]

Moving the check into self._obj.decompress raises a whole slew of other errors. Need to really step through the code to resolve it I think - something screwy is afoot.

[bryanhelmig]

I hope no-one minds a friendly "bump" here. Happy to chase down any concerns.

[sigmavirus24]

I don't mind @bryanhelmig =). I responded to your inline comment with a question of my own. I hope it helps.

[bryanhelmig]

Looks like Python 3 failed some tests - I'll have to chase those down later after I get my Python 3 env in shape again.

[shazow]

Big thanks to everyone for keeping this moving in my semi-absence. :) Will be back with viable internet in a few days.

[sigmavirus24]

That's an interesting way of handling the problem. I wonder if those test failures are intermittent failures.

[bryanhelmig]

@sigmavirus24 yeah, I'm thinking the errors are intermittent. Can we retest?

WRT the PR - it's a bit different. It's easy to see why the error is raised at the lowest level:

>>> import zlib
>>> compress = zlib.compressobj(6, zlib.DEFLATED, 16 + zlib.MAX_WBITS)
>>> data = compress.compress(b'foo')
>>> data += compress.flush()
>>> d = zlib.decompressobj(16 + zlib.MAX_WBITS)
>>> d.decompress(data)
'foo'
>>> d.decompress(b'') # run all you like...
''
>>> d.flush()
''
>>> d.decompress(b'') # raises after flush in python 2.7/3.2 - but succeeds in pypy (??)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
zlib.error: Error -2 while decompressing: inconsistent stream state

It might be "more proper" to track the state of the decompressobj vs. ignoring empty data.

[shazow]

Hmm yes, those intermittent failures are annoying. Looks good now.

@bryanhelmig I like your solution, MERGING! Thank you. 🍒

[bryanhelmig]

❤️ @shazow :-)