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

HttpLoggingInterceptor hangs when response uses "Transfer-Encoding: chunked" #6601

Closed
fabiohecht opened this issue Mar 22, 2021 · 6 comments
Closed
Labels
bug Bug in existing code

Comments

@fabiohecht
Copy link

Hi, I have a server that responds with "Transfer-Encoding: chunked" and does not close the connection. Without using the HttpLoggingInterceptor with BODY level, execute() returns a partial response with each chunk. As soon as I enable logging with BODY level, the HttpLoggingInterceptor hangs on source.request(Long.MAX_VALUE) // Buffer the entire body.. I did not expect that enabling logging should have an effect on how the client works.

@fabiohecht fabiohecht added the bug Bug in existing code label Mar 22, 2021
@yschimke
Copy link
Collaborator

Yep - this is a limitation of the HttpLoggingInterceptor, we are unlikely to fix this. If you need something more specific you should copy and alter it's implementation to your requirements.

@merlinran
Copy link

merlinran commented Dec 16, 2022

Alas, spent hours trying to figure out why a pretty normal client.newCall(request).execute() blocks forever when connecting to a Server-Sent Event endpoint (meant to use okhttp-sse).

I'm relatively new to the Java/Kotlin ecosystem so could be wrong, but feels to me it's not particularly hard to replace source.request(Long.MAX_VALUE) with a forwarding source like what this Progress.java recipe shows, but duplicating the bytes instead. Can try playing with it if no objection.

Or at least add a few words in the comment for Level.BODY to remind the existence of the trap.

@JakeWharton
Copy link
Member

That means multiple response bodies can now have their parts interleave in the logs. It also means you may split across a codepoint or grapheme cluster making the rendering inaccurate.

@merlinran
Copy link

merlinran commented Dec 16, 2022

good point. maybe buffer the duplicated bytes to up to 100kB or until the source closes? In practice I don't think someone would rely on logs to verify the response body larger than 100kB. Feels to me it's pretty important to prevent a utility in this library from violating the core assumption - execute() doesn't have to wait for the full response body.

@yschimke
Copy link
Collaborator

We don't actively enhance this functionality or make it more configurable. We might be best just disabling it for these types of uses based on content type as its clearly broken if causing delays. I'll follow up with a PR to do that.

To unblock you, we've previously suggested it's worth copy pasting the implementation to make it do exactly what you need.

@merlinran
Copy link

Nice solution! This is no longer an issue to me once the cause was found, but it may help others from entering similar situations. Thank you!

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

No branches or pull requests

4 participants