Add stored cookies without rebuilding the request - #2384

Summary

When the client's cookie store has a cookie for the request URI, DefaultAsyncHttpClient.executeRequest adds it with request.toBuilder(), addCookieIfUnset and build(). That rebuild copies every header into a new header set, copies the cookie, form param and body part lists twice and encodes the URI again, on every request that carries a session cookie.

The URI step is also a bug. toBuilder() always uses the fixing URI encoder, so a request built with URL encoding disabled is re-encoded, but only when the store has a cookie for it:

Request request = new RequestBuilder("GET", true).setUrl("http://localhost/a|b?q={x}").build();
// no stored cookie:   http://localhost/a|b?q={x}
// with stored cookie: http://localhost/a%7Cb?q=%7Bx%7D   (on main)

Changes:

1. DefaultRequest.withCookies (package-private) returns a request that differs only by its cookie list. The URI is kept as built and the other fields are shared, not copied.
2. executeRequest merges the stored cookies into a new list with the same rule as addCookieIfUnset: the request's own cookies first and never replaced, then each stored cookie whose name is not there yet, so the first stored cookie of a name wins. If every stored name is already set, the request is passed on unchanged. A null cookie list (possible through the public DefaultRequest constructor) is treated as empty, as the builder did.
3. Other Request implementations, including subclasses of DefaultRequest, still go through toBuilder(), as they may override what it copies.
4. StoredCookieRequestBenchmark measures executeRequest up to the request filters with and without a matching stored cookie.

Behaviour notes

• Same cookies, in the same order, as before, with names compared case-sensitively as addCookieIfUnset does. The only intended difference is that the URI is no longer re-encoded on this path.
• The new request shares the original's HttpHeaders object and lists. Without a matching stored cookie (or with the cookie store disabled) executeRequest already passes the caller's request, with the same objects, to the filters and the sender, so the copy was incidental to the cookie path. No production code mutates request.getHeaders(): the redirect and auth interceptors copy first. Before this change the rebuild hid a missing copy for every request with a stored cookie, so a test now executes one request twice across a cross-origin 302 with a stored cookie and checks that its headers are unchanged.
• No public API change: withCookies is package-private, the new helpers in DefaultAsyncHttpClient are private. Revapi passes.

Not changed here: these paths also go through toBuilder() and so still re-encode a URI built with URL encoding disabled. They are not on the per-request path and change more than the cookies, so they are left for a separate change:

• preProcessRequest, for a request with a range offset, and ResumableAsyncHandler.adjustRequestRange;
• the retry built by Unauthorized401Interceptor and ProxyUnauthorized407Interceptor;
• the target request rebuilt by ConnectSuccessInterceptor after a proxy CONNECT.

So a request built with URL encoding disabled keeps its URI when a stored cookie is added, but not yet on an auth retry or through a tunnelling proxy.

Benchmark

StoredCookieRequestBenchmark, JMH 1.37, JDK 17.0.2, Windows, -prof gc, 5 x 1 s warmup, 5 x 1 s measurement, main against this branch. A request filter ends each call by throwing a preallocated exception, so nothing reaches the network; both arms then build the same failed future. noStoredCookie sends to a host with no stored cookie and is the baseline.

Bytes/op with escape analysis off (-XX:-DoEscapeAnalysis, 1 fork), which gives exact allocation counts:

Adding the stored cookie costs 920 to 1400 bytes on main, growing with the header count, and 312 bytes here, independent of it (the store lookup's list, the merged cookie list and the new request).

With default JVM flags (2 forks) escape analysis removes part of it, so the figures vary by a few dozen bytes between forks: storedCookie 1800 -> 1056 B/op with 4 headers and 2184 -> 1272 B/op with 16. The host was running other builds, so the timings had error bars of 50 to 100 percent and are not reported.

Tests

New tests in DefaultAsyncHttpClientTest capture the request that reaches the filters, with no server:

• a request built with URL encoding disabled keeps its URL (fails on main with the re-encoded URL above);
• request cookies are not replaced by stored ones, and the original request is not modified;
• a request that already has a cookie of every stored name is passed on as is;
• the first stored cookie of a name wins;
• cookie names are compared case-sensitively (SID on the request and sid in the store both go out);
• every other field of DefaultRequest is carried over as the same object, checked reflectively on requests that set every field to a non-null value no other field holds, except the Boolean flags; across three such requests each flag takes both values and each pair of flags differs, so a swapped flag or a constant in its place fails;
• a subclass of DefaultRequest goes through its toBuilder();
• a DefaultRequest with a null cookie list gets the stored cookies.

RedirectCredentialSecurityTest.crossDomainRedirectLeavesCallerRequestHeadersUnchanged executes one built request with Authorization and Cookie headers and a stored cookie twice across a cross-origin 302, and checks after each run that the request's headers are unchanged, that the stored cookie was sent (so the run went through withCookies) and that both runs sent the credentials to the first origin only.

./mvnw clean verify -Dgpg.skip=true passes on JDK 17 (2001 tests; no JDK 11 and no gpg on the host); CI covers JDK 11.

Second-agent review

Both reviewers ran from fresh CLI sessions, read-only, with the intent and constraints in the prompt.

Six rounds in all: three before the PR was opened (15ece6b, 3c843f5, f7ff016), and three on the commit that answers the maintainer review (rounds 4 to 6; it was amended before the push after rounds 4 and 5). Fixed from earlier rounds:

• the field test set all four Boolean flags to true, so a swap of two of them in withCookies would pass; now two requests make every pair differ (checked by swapping two flags, which fails the test), and the body fields are compared too;
• added tests for the subclass path and for the first stored cookie of a name;
• a DefaultRequest with a null cookie list failed only on the stored-cookie path; it is now treated as empty;
• round 4 (independent reviewer, nit): the redirect test did not check that the stored cookie was added, so it could silently stop going through withCookies; it now asserts that the cookie reached the first origin;
• round 5 (independent reviewer, nit): both requests in the field test had the same followRedirect and refuseCrossOriginBodyOnRedirect, so passing Boolean constants for those two would pass; a third request now gives each flag both values.

Not changed:

• Shared HttpHeaders and lists (round 1, guided reviewer as a bug, independent reviewer as a nit). A filter or a reused RequestBuilder that mutates them now reaches the original request also when a stored cookie matched. The path without a matching stored cookie already behaves that way, as described under behaviour notes, and deep-copying the headers is the cost this change removes.
• Normalisation that build() did for a hand-built DefaultRequest (round 1, independent reviewer, nit), such as a null charset. Such a request fails the same way without a stored cookie.

Claude Code on behalf of Pavel Ptashyts

🤖 Generated with Claude Code