Chucker 3.1.2 re-writing the Server Response; throws IOException when used as a networkInterceptor

[jannisveerkamp]

✍️ Describe the bug

After updating to 3.1.2 (from 3.0.1) every request fails with an IOException:

java.io.IOException: ID1ID2: actual 0x00007b22 != expected 0x00001f8b
System.err:     at okio.GzipSource.checkEqual(GzipSource.java:205)
System.err:     at okio.GzipSource.consumeHeader(GzipSource.java:120)
System.err:     at okio.GzipSource.read(GzipSource.java:73)
System.err:     at okio.RealBufferedSource.read(RealBufferedSource.java:51)
System.err:     at okio.ForwardingSource.read(ForwardingSource.java:35)
System.err:     at retrofit2.OkHttpCall$ExceptionCatchingResponseBody$1.read(OkHttpCall.java:288)
System.err:     at okio.RealBufferedSource.select(RealBufferedSource.java:100)

Chucker 3.0.1 returned the original Response. 3.1.x seems to process the response and returns this "processed Response" (see here).

I see the need of processing the Response and doing stuff with it. But the Interceptor has to return the original response at the end, since Chucker is only some kind of debugging ouput, which should not change the Request/Response in any way.

💣 Steps to reproduce

• Add ChuckerInterceptor as a networkInterceptor
• I think the Request has to be gzipped

🔧 Expected behavior

• Chucker should not alter the Server Response

📱 Tech info

Chucker version: 3.1.2
OkHttp version: 3.14.6

📄 Additional context

This only happens when the ChuckerInterceptor is added as a networkInterceptor.
The Notification shows the correct Request with the Response.

[vbuberen]

Thanks for your report.

@cortinico the more such reports we have the more I want to rework interceptor in a way where we just do a copy of response for processing, while returning the original response further to the chain. Something like I showed you when we discussed the issue presented in 3.1.0.
Because it seems the most viable way since we could be sure that we aren't altering the original response in the chain.

[cortinico]

@cortinico the more such reports we have the more I want to rework interceptor in a way where we just do a copy of response for processing, while returning the original response further to the chain. Something like I showed you when we discussed the issue presented in 3.1.0.

Agree. Maybe we should spend some time writing some proper JUnit tests for the critical ChuckerInterceptor sections before doing further changes?
Moreover the sample app and the suggested setup is to use Chucker as Application Intercptor.

I'm curious to hear @jannisveerkamp what's your use case to use Chucker as networkInterceptor? Any reason why you're not using it as application interceptor?

[jannisveerkamp]

Is there actually a reason to not return the original Response at the end?

I just want to see every outgoing Request. When adding Chucker as an appInterceptor I also see Requests running into a max-age Cache for example. Furthermore I have a Usecase where I only fetch a Response from the Cache, which I also don't want to see.

[vbuberen]

Hi @jannisveerkamp
No, there is no reason. It just was like that historically and I joined the project pretty recently, so when I arrived it already was like that.

Your issue isn't the first of such type. For example, here is #203 , #225, which I believe are all from the same origin, so I am going to take action and do it described way.

@cortinico Absolutely agree about test coverage. Since it appears again after our recent discussion in Slack, it is not not nice to have, but must to have priority from my point of view.

The only thing is that I am a bit overwhelmed at the moment and will be available in ~2 weeks to do this thing (hopefully, earlier). So attaching help-wanted in case somebody wants to pick it up by that time.

[MiSikora]

In my intial #226 PR I did changes where requests and responses were gzipped back again before being sent or received respectively. I shied away from it so the behaviour wouldn't be changed.

Ideally, I think, Chucker should intercept raw data directly to files instead of runtime memory and do the work on these files as input streams. This would allow data to be streamed to both end consumer and a file. It would also help with large requests or responses and potentially some OOM issues.

[vbuberen]

Hey @jannisveerkamp
Could you test the snapshot version with possible fix from #250 ?
Would really appreciate it.
You can try it out by using the following dependency version:
implementation 'com.github.MiSikora.chucker:library:body-re-write-fix-SNAPSHOT'
In case you want to check other commits you can get proper dependency name on Jitpack:
https://jitpack.io/#MiSikora/chucker/body-re-write-fix-SNAPSHOT

[cortinico]

Ideally, I think, Chucker should intercept raw data directly to files instead of runtime memory and do the work on these files as input streams.

Can you elaborate more on this? Are you suggesting we first dump the raw data to file and then we move it over inside the DB?

[MiSikora]

Not exactly. I suggest that Chucker should stream network inputs and outputs to files while it streams also further down the chain. Then, once the file is ready, you can do whatever you want with it like it is done now in the intercept() method. If files get too big during downstreaming you can always shortcircuit them. It would work more or less in the same way as Relay and Cache from OkHttp. Whatever happens after the file is processed is up to the implementation. It can be moved to DB as a blob and deleted or you can store a handle to the file, etc.

[cortinico]

It would work more or less in the same way as Relay and Cache from OkHttp

The idea sounds nice. If you would be able to send a draft PR for this approach we could brainstorm on this (or you can join the #chucker channel on Kotlinlang and we can discuss over there)

[jannisveerkamp]

I can test it later or tomorrow. But I still think it should return the original response 💭

[vbuberen]

Thanks. No rush here.
As to your opinion on returning the original response - completely agree with you.
I am going to implement it, but later when have more free time.

[MiSikora]

Ok, I pushed one small change to the PR to not rewrite the response at all.

The idea sounds nice. If you would be able to send a draft PR for this approach we could brainstorm on this (or you can join the #chucker channel on Kotlinlang and we can discuss over there)

I'll gladly make some PoC when I find more time as this is rather big change.

[MiSikora]

Hey, are there plans to include this in 3.2.0?

[cortinico]

Hey, are there plans to include this in 3.2.0?

Ideally yeah. We don't have a ETA yet, but this is definitely something worth to end up in the upcoming release.

[vbuberen]

Yes, we should include it into 3.2.0.
As for ETA - I would say that I would love to see support of GraphQL to release the new version.
It seems that I need to dive into that old PR myself, since there is completely no progress or movement for quite a long time.

[cortinico]

As for ETA - I would say that I would love to see support of GraphQL to release the new version.

Definitely valuable but not a release blocker IMHO.

[vbuberen]

Yeah, it is not a blocker, but it was postponed quite a lot as I see it.
Without GraphQL it will be not 3.2.0, but something like 3.1.3, since there were no serious changes, but lots of fixes.