avoid duplicate Content-Length header in DefaultClient - #3451
Conversation
|
any update? |
|
Thanks for the fix and the wire-level regression test! We ended up merging #3448, which removes the manual Content-Length write from the header loop entirely and lets the JDK own the header — that also covers the bodyless-request duplicate this PR guards against (the "0" fallback now fires exactly once). Holding off on merging this one to avoid stacking two untested-in-combination changes to the same code path; if you think the containsKey guard is still needed on top of #3448, happy to revisit. |
|
Checked this against current repro: body-less So agreed, the guard isn't needed any more. The header loop no longer writes |
velo
left a comment
velo
left a comment
There was a problem hiding this comment.
Two things here: the branch is out of date, and I think the fix targets the wrong line.
The branch conflicts
DefaultClient#convertAndSend was reworked on master by #3507 (issue 2068 — empty body content type). The if (body == null && request.httpMethod().isWithBody()) block this patch edits is now } else if (request.httpMethod().isWithBody()) { with a longer explanatory comment. Please rebase onto master.
The duplicate can't happen with canonical casing
Look at the header loop just above (DefaultClient.java:170-188):
for (String value : request.headers().get(field)) {
if (field.equals(CONTENT_LENGTH)) {
if (!gzipEncodedRequest && !deflateEncodedRequest) {
contentLength = Integer.valueOf(value);
}
}
...When the field is Content-Length, the value is captured into the local contentLength and never forwarded via addRequestProperty. So for a user-supplied @Headers("Content-Length: 0") on a body-less request there is exactly one Content-Length on the wire — the one added at the end. Guarding that final addRequestProperty with containsKey(CONTENT_LENGTH) would drop the header entirely rather than de-duplicate it.
The duplicate you're seeing almost certainly comes from non-canonical casing. request.headers() is a String.CASE_INSENSITIVE_ORDER TreeMap (RequestTemplate.java:867) that preserves the caller's original casing, but line 175 compares with equals. So @Headers("content-length: 0") misses that branch, falls through to addRequestProperty("content-length", "0"), and then line 228 adds Content-Length: 0 — two headers.
Suggested fix
Change line 175 to field.equalsIgnoreCase(CONTENT_LENGTH). That fixes the actual duplicate, keeps the existing "let HttpURLConnection own the header" behaviour, and also fixes the related bug where a lower-cased Content-Length isn't picked up for setFixedLengthStreamingMode under disableRequestBuffering.
Test coverage
contentLengthHeaderIsNotDuplicatedForBodylessRequest is gated on @EnabledIfSystemProperty(named = "sun.net.http.allowRestrictedHeaders", matches = "true"), so it is skipped in CI as things stand — the regression it guards wouldn't be caught. Please either arrange for that property to be set for this test class, or write the assertion so it works without restricted headers enabled.
9e4e847 to
982fc33
Compare
|
Good catch on the casing, that's exactly it. With canonical casing the value is captured and never forwarded, so there's no duplicate; a lower-cased repro: body-less For CI coverage I used the Full |
2 participants
Repro: a request whose template carries a non-canonically cased
content-lengthheader. For a body-lessPOSTwithsun.net.http.allowRestrictedHeaders=true, the header loop forwardscontent-lengthviaaddRequestPropertyand the body-less fallback then addsContent-Length: 0, so the field goes out twice. RFC 7230 §3.3.2 forbids generating multipleContent-Lengthfields; the duplicate is ambiguous framing and servers reject it (Tomcat returns400, #2862). The same missed comparison means a lower-casedcontent-lengthon a request with a body is not picked up forsetFixedLengthStreamingModeunderdisableRequestBuffering, so the request goes out chunked instead of fixed-length.Cause: the header loop compares with
field.equals(CONTENT_LENGTH), butrequest.headers()preserves the caller's original casing.Fix: compare with
equalsIgnoreCase, so any casing is captured into the localcontentLengthand never forwarded; the JDK stays the only writer of the header.lowerCaseContentLengthHeaderIsUsedForFixedLengthStreamingModeruns ungated in CI: fixed-length streaming mode makes the JDK emit the header itself, so no restricted-headers flag is needed. Before the fix the request goes out chunked with noContent-Length; after, exactly one.contentLengthHeaderIsNotDuplicatedForBodylessRequestcovers the wire-level duplicate from #2862; it stays gated onsun.net.http.allowRestrictedHeaderssince the duplicate only reaches the wire with that flag set.