[pavel-ptashyts]

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. The only intended difference is that the URI is no longer re-encoded.
• 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.
• No public API change: withCookies is package-private, the new helpers in DefaultAsyncHttpClient are private. Revapi passes.

Not changed here: preProcessRequest (for a request with a range offset) and ResumableAsyncHandler.adjustRequestRange also go through toBuilder() and so re-encode a RAW URI. They are not on the per-request path and have to change a header anyway, so they are left for a separate change.

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:

Arm	Headers	main	This PR
storedCookie	4	1928	1320
storedCookie	16	2408	1320
noStoredCookie	4	1008	1008
noStoredCookie	16	1008	1008

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;
• every other field is carried over, with two requests chosen so that swapping any two Boolean flags fails;
• a subclass of DefaultRequest goes through its toBuilder();
• a DefaultRequest with a null cookie list gets the stored cookies.

./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.

Reviewer	Agent and model	Effort	Commit reviewed	Result
Guided (against AGENTS.md, on git diff upstream/main...HEAD)	Codex gpt-6-astra	medium	f7ff016	No findings
Independent (no checklist, reads the files)	Claude Code claude-opus-5-5	medium	f7ff016	No findings

Three rounds in all (15ece6b, 3c843f5, f7ff016). 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.

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