-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KTOR-9396: Fix Java HTTP client not sending Content-Length header for GET requests with no body #5429
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
Open
EugeneScully
wants to merge
3
commits into
ktorio:main
Choose a base branch
from
EugeneScully:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
KTOR-9396: Fix Java HTTP client not sending Content-Length header for GET requests with no body #5429
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
Doesjava.net.http.HttpRequest.newBuilder(URI)default to the GET method, and does omittingHttpRequest.Builder.method(String, BodyPublisher)cause HEAD/OPTIONS/TRACE requests to be sent as GET in Java 11+ HttpClient?💡 Result:
Yes.
HttpRequest.newBuilder(URI)(andnewBuilder()) produces a builder whose default HTTP method is"GET"unless you explicitly set another method. (docs.oracle.com)HttpRequest.Builder.method(String, BodyPublisher)(or the convenience setters like.POST(...),.PUT(...),.DELETE(),.GET()) does not cause Java’sHttpClientto “reinterpret” a request asHEAD/OPTIONS/TRACE. It simply remains a GET request. (docs.oracle.com).method("HEAD", BodyPublishers.noBody()), etc.). (docs.oracle.com)Citations:
🏁 Script executed:
Repository: ktorio/ktor
Length of output: 766
🏁 Script executed:
Repository: ktorio/ktor
Length of output: 2218
🏁 Script executed:
rg "supportsRequestBody" -A 2 -B 2 --type ktRepository: ktorio/ktor
Length of output: 81
🏁 Script executed:
rg "supportsRequestBody" -A 2 -B 2Repository: ktorio/ktor
Length of output: 7038
🏁 Script executed:
rg "REQUESTS_WITHOUT_BODY" -A 3 -B 1Repository: ktorio/ktor
Length of output: 596
🏁 Script executed:
Repository: ktorio/ktor
Length of output: 1096
Always set the HTTP method here.
On line 54, when
method.supportsRequestBodyis false (for HEAD, OPTIONS, TRACE) andbodyisOutgoingContent.NoContent, the conditional skips callingbuilder.method(...), leaving the builder at its default GET method. This causes HEAD/OPTIONS/TRACE requests to be sent as GET instead. Separate the method assignment from the body publisher selection: always callmethod(method.value, bodyPublisher)unconditionally, varying only the body publisher between the actual content andHttpRequest.BodyPublishers.noBody().Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I apply that change, then the
testGetRequestDoesNotSendContentLengthHeadertest fails.HttpRequest.BodyPublishers.noBody()still sets content-length=0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EugeneScully, you're absolutely right — I apologize for the incorrect suggestion. This is a well-known JDK issue.JDK-8283544 addressed the original
Content-Length: 0problem for GET requests, but the follow-up JDK-8358942 confirms that using.method("GET", BodyPublishers.noBody())still reproduces the issue. The root cause is in the JDK's HTTP/1.1 implementation: ifcontentLength == 0, it unconditionally setsContent-Length: 0.So
BodyPublishers.noBody()hascontentLength()returning0, which always triggersContent-Length: 0— your test correctly catches this, and my suggestion was wrong.However, there is still a residual concern with the current PR: for HEAD and OPTIONS requests with no body,
method.supportsRequestBodyisfalseandbodyisOutgoingContent.NoContent, sobuilder.method(...)is never called. Since the builder default isGET, HEAD and OPTIONS requests would be silently sent as GET.A cleaner fix that handles GET correctly while still setting the method for HEAD/OPTIONS would be:
Or more concisely:
This scopes the JDK workaround specifically to GET with no body, while ensuring HEAD/OPTIONS/TRACE still have their method set. Would you like to address the HEAD/OPTIONS case, or is that out of scope for this PR?
✏️ Learnings added
🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should probably also handle the HEAD, OPTIONS, TRACE scenarios with no body.
I'm happy to add test cases for those.
They should omit the content-length header if the client has no body according to RFC 9110 shouldn't they?
We should review the original apache & android client fixes for this issue here and keep this fix similar : #4895
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Let me look at PR
#4895and thesupportsRequestBodydefinition to give you a well-informed answer.🏁 Script executed:
Length of output: 53
🏁 Script executed:
Length of output: 2483
🏁 Script executed:
Length of output: 1794
🌐 Web query:
💡 Result:
Content-Length, but must not unless the value equals the number of octets that would have been sent in the body of a correspondingGETresponse. Also, for HTTP/1.1 framing, any response toHEAD“cannot contain a message body”. [1][2]OPTIONSresponses may have “content, if any”. If you send no content, you may omitContent-Lengthor sendContent-Length: 0(for a 200). However, if you choose status 204, the server must not sendContent-Lengthat all. [3][1]TRACErequest; consequently it generally should not sendContent-Lengthon a TRACE request (and if it does, it should only ever be consistent with “no content”, i.e., 0). [4][1]TRACEnormally returns the received request as the content of a200response; if you instead return no content, the same rules as above apply (e.g.,Content-Length: 0allowed on 200; forbidden on 204). [4][1]Sources
[1] RFC 9110 (HTTP Semantics), Content-Length rules (incl. HEAD/1xx/204/304) https://datatracker.ietf.org/doc/rfc9110/
[2] RFC 9112 (HTTP/1.1), message body length rule for HEAD responses https://www.rfc-editor.org/rfc/rfc9112.html
[3] RFC 9110, §9.3.7 OPTIONS (“content, if any”) https://datatracker.ietf.org/doc/rfc9110/
[4] RFC 9110, §9.3.8 TRACE (TRACE request must not include content; TRACE response reflects request as content) https://datatracker.ietf.org/doc/rfc9110/
Let me look at the actual Apache and Android engine implementations for comparison.
🏁 Script executed:
Length of output: 10995
@EugeneScully, great question — and you're right to align this with PR#4895. Here's the full picture:Apache's approach (in
ApacheRequestProducer.kt) is identical in intent — only set the entity whenmethod.supportsRequestBody || hasContent. The key difference is that Apache'sRequestBuilder.create(method.value)sets the HTTP method and entity separately, so skipping the entity still sendsHEAD/OPTIONS/TRACEwith the correct method.The Java
HttpRequest.BuilderAPI doesn't have that luxury. The HTTP method andBodyPublisherare always set together viabuilder.method(String, BodyPublisher). Not calling it at all defaults toGET. So the current PR's fix — while correct forGET— introduces a regression forHEAD/OPTIONS/TRACEwith no body: they'd now be silently sent asGET.@EugeneScully, you're right on both counts — RFC 9110 and alignment with PR#4895.RFC 9110 §8.6 stance: A user agent SHOULD NOT send
Content-Lengthin a request when there is no body and the method semantics do not anticipate one. This applies toHEAD,OPTIONS, andTRACEwith no body, not justGET. And RFC 9110 §9.3.8 goes further — a client MUST NOT send content in aTRACErequest.The Apache comparison is instructive but not directly equivalent. Looking at
ApacheRequestProducer.kt, it uses the same guard (method.supportsRequestBody || hasContent) to decide whether to setBasicHttpEntity. However, Apache'sRequestBuilder.create(method.value)sets the HTTP method independently of the entity — so skipping the entity still sends aHEADwith the correct method.The Java
HttpRequest.BuilderAPI couples method and body publisher together. There is no separate setter for the method string; the only way to set the HTTP method (beyond GET/POST/PUT/DELETE) isbuilder.method(String, BodyPublisher). So the current PR's condition means that forHEAD/OPTIONS/TRACEwith no body,builder.method()is never called and the builder silently defaults toGET— a regression from the original code.The JDK root cause (from JDK-8283544): the
Http1Request.headers()code unconditionally sendsContent-Length: 0whencontentLength == 0.contentLength()returning less than zero means unknown content length, which triggersTransfer-Encoding: chunkedinstead — also undesirable for bodyless methods.Practical recommendation:
Scope the "skip
builder.method()" workaround toGETonly. ForHEAD/OPTIONS/TRACEwith no body, still callbuilder.method()(correctly sets the method); those methods will still be affected by the JDK Content-Length: 0 bug, but that's a separate, pre-existing JDK issue — and is far better than sending them asGET. The condition change would look like:On tests: Yes, absolutely add tests for
HEAD/OPTIONS/TRACEwith no body. They should at minimum assert the correct HTTP method is used. Asserting the absence ofContent-Length: 0for those methods would be ideal per RFC 9110, but it may fail due to the JDK bug on older JDK versions — you might want to scope that assertion or file it as a known JDK limitation.✏️ Learnings added
🧠 Learnings used