Fix chunked decompression logic #747
Conversation
|
Missing coverage. Also do we need that extra _decode? Should we re-order the loop instead? |
|
Yeah, I meant to write a thing about that but then...I didn't. I don't know why. What I'm not sure about is whether that last flush is needed at all. I've yet to be able to construct a scenario where we need it. You're right though, we might be able to resolve this problem by re-ordering the loop, let me have a think. |
|
Hmm. The way the loop is written makes it really hard to have a single decode point. In particular, there's a lot of state being hung off the object. This means, for example, we cannot safely call However, we can refactor to pull the flush logic out of |
|
So I can do that refactor, but we still don't hit the line in my testing. We're at a stage now where I have to investigate exactly how flush behaves in all the cases we use it. |
|
AHA! I got it! We don't need to call
So, on CPython (and probably PyPy, I'll have to check), we definitely don't need it at all, in any circumstance. That leaves me with a question: what does Jython need? |
|
Just did a source code check: PyPy also always calls the decompressor with Z_SYNC_FLUSH from the decompressobj, so we don't need it there either (good work PyPy!). That potentially leaves Jython as the odd-one-out. If we can prove that Jython doesn't automatically use Z_SYNC_FLUSH, then this is a bug worth reporting to them @sigmavirus24. |
|
And I just walked straight into a Jython zlib bug unrelated to this one. |
|
For those who care, the bug is here. However, on the flush issue, I haven't been able to demonstrate that Jython needs it. However, that doesn't prove much: it may need it and I just haven't spotted it yet. I need to dive deeper, sadly. |
|
Ok, so sadly Jython does not pass Z_SYNC_FLUSH, it passes Z_NO_FLUSH. I don't know if this is a problem or not, because I don't know what zlib does with this information: I'll see if I can find out. Regardless, I've mentioned this inconsistency as Jython bug 2434. |
|
🔩 |
|
So here's the input from the Jython side - and really from the underlying Java support - since Java 7, Z_SYNC_FLUSH is supported More in this Java dev blog post So we can readily support in Jython. Thanks for filing that bug with us! |
|
Right, so in the meantime we need to decide what we're doing while Jython is suspected to require flushing. @shazow, got a preference? |
|
What are the options? Always flush vs what? What's the down-side of always-flush in this case? |
|
The only downside of always flush is testing-based: specifically, I'm currently unable to construct a test-case that hits the flush statement. |
|
I'm alright with doing the |
|
Ok, cool, I'll fix that up. |
|
Hmm, why won't Travis build this? |
|
Ok, now you build. Stupid Travis. |
|
👍 |
| # decoder. However, on Jython we *might* need to, so | ||
| # lets defensively do it anyway. | ||
| decoded = self._flush_decoder() | ||
| if decoded: # Platform-specific: Jython. |
There was a problem hiding this comment.
Hm why is the yield Jython-specific? (Why does Jython yield an extra thing?)
(Should the parent if-block be the Platform-specific?)
There was a problem hiding this comment.
Because the flush works on all platforms, but it doesn't return anything on any other platform (the decoder was flushed at the last decompress call). The if block is not platform specific because decode_content may be True on any platform.
Fix chunked decompression logic (Jython bugfix)
|
Just closed out the Jython related bug: http://bugs.jython.org/issue2434 - we should have compliant support, specifically all flush behaviors, thanks to using the underlying support available as of Java 7 (which Jython 2.7 requires). I don't think you need to do anything specific for Jython going forward, although this will of course require >= Jython 2.7.1 |
|
Yay this is excellent news, we should add Jython on our list of supported platforms in the README. |
|
Wonder how hard it would be to setup a jython 2.7.1 endpoint for travisci... |
Resolves #743.