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.
2 participants
Repro: send a body-less
POST/PUTwhose template already carries aContent-Lengthheader (e.g.@Headers("Content-Length: 0")), withsun.net.http.allowRestrictedHeaders=trueso the header reaches the wire.Cause:
DefaultClient.convertAndSendwrites the requestContent-Lengthin the header loop and then unconditionally adds a secondContent-Length: 0for every body-less method that allows a body, 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).Fix: only add the fallback
Content-Length: 0when the request does not already declare aContent-Length.The regression test is gated on
sun.net.http.allowRestrictedHeaderslike the existingContent-Lengthtests, sinceHttpURLConnectionotherwise drops the restricted header, and asserts the field is sent once.