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

Data not cached when gzip content-encoding is combined with chunked transfer-encoding. #33

Open
DanielLukic opened this issue Mar 10, 2013 · 5 comments

Comments

@DanielLukic
Copy link

See line 68 in ChunkedInputStream:

    if (bytesRemainingInChunk == 0 && in.available() >= MIN_LAST_CHUNK_LENGTH) {

Using available() on an InputStream is generally a bad idea, I guess. In many cases this will return 0.

In case of a server returning gzipped data (content-encoding: gzip) of more than a few KB, it will probably turn on chunked encoding (transfer-encoding: chunked). Most probably to support non-buffered streaming mode.

In this case the ChunkedInputStream will never commit the response data to the cache.

The easy fix is probably to reduce the if statement in line 68 to:

    if (bytesRemainingInChunk == 0) {

Does that make sense to anyone? ^^

Cheers,

dl

@lkorth
Copy link

lkorth commented Mar 11, 2013

+1 this solves issue #32. Do we need to open a bug with Google and get this committed upstream? I had the same issues using the HttpResponseCache built in to Jelly Bean.

@DanielLukic
Copy link
Author

Do it! ^^

But I am not 100% convinced that this is a proper fix. Because we could create an IOException when reading ahead. So a proper fix probably needs to be a bit more sophisticated.

In a project I am working on right now I fixed this issue by moving the "read ahead" step into the close() method. Ignoring all exceptions. Not sure if this is any better.

Well, would not hurt to report this to Google, I guess. So please go ahead, if that is OK with you. I would not really know where to go.. ^^

dl

@lkorth
Copy link

lkorth commented Mar 11, 2013

I have a feeling that a more complex fix is the right way to go, but this works for now for development.

@candrews thoughts?

@candrews
Copy link
Owner

A bug should definitely be opened with the Android project about this.

@swankjesse can you chime in? You wrote this, and presumably this effects some of the projects you're currently working on :)

@swankjesse
Copy link

Fixed in OkHttp! See Android issue 38817.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants