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

HttpContent is encoded two times when posting data #648

Closed
chingor13 opened this issue May 24, 2019 · 4 comments · Fixed by #910
Closed

HttpContent is encoded two times when posting data #648

chingor13 opened this issue May 24, 2019 · 4 comments · Fixed by #910
Assignees
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@chingor13
Copy link
Collaborator

The resulting request is correctly encoded once, but we are encoding an extra time when we calculate the content length.

@chingor13 chingor13 added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 24, 2019
@chingor13 chingor13 self-assigned this May 24, 2019
@sduskis
Copy link
Contributor

sduskis commented May 29, 2019

This is a duplicate of #283.

@chingor13
Copy link
Collaborator Author

It turns out this is non-trivial to fix. The issue is that for some transport adapters, we have to set the Content-Length header before we start streaming the content. This means that we must count the bytes before we send them. We don't want to cache the encoded content in memory as it defeats the purpose of streaming the content and could cause memory spikes in the application. We don't want to write to a temporary file as there may be file system issues/restrictions (like on GAE) that we cannot necessarily plan for.

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 8, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 18, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 28, 2019
@codyoss
Copy link
Member

codyoss commented Oct 15, 2019

@chingor13 do we have more details on this one? Such as what is being affected? Or is there a finite list of Transport Adapters that need the Content-Length set?

@codyoss codyoss assigned codyoss and unassigned chingor13 Dec 9, 2019
@codyoss
Copy link
Member

codyoss commented Dec 9, 2019

I will take a stab at this.

codyoss added a commit to codyoss/google-http-java-client that referenced this issue Dec 10, 2019
Currently any time HttpRequest works with encoded data it encodes
the data twice. Once for the actaul stream and once for checking
the length of the stream. Instead, when there is encoding just don't
set the content length. This will cause the underlying transports,
with a few tweaks, to use Transfer-Encoding: chunked.

Fixes googleapis#648
codyoss added a commit that referenced this issue Dec 17, 2019
Currently any time HttpRequest works with encoded data it encodes
the data twice. Once for the actual stream and once for checking
the length of the stream. Instead, when there is encoding just don't
set the content length. This will cause the underlying transports,
with a few tweaks, to use Transfer-Encoding: chunked.

Fixes #648
clundin25 pushed a commit to clundin25/google-http-java-client that referenced this issue Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
4 participants