Optimize response header handling for HTTP/2 (P2) by xinlian12 · Pull Request #48463 · Azure/azure-sdk-for-java

xinlian12 · 2026-03-18T18:03:41Z

Option A: Skip redundant toLowerCase() for HTTP/2 headers

Add keysAlreadyLowerCase flag to Cosmos HttpHeaders
Detect HTTP/2 in ReactorNettyHttpResponse.headers() via version check
Saves ~15-25 toLowerCase calls + String allocations per response

Option C: Eliminate toLowerCaseMap() as redundant

Change set() to store lowercased key in HttpHeader (not original case)
toMap() now returns lowercase keys, making toLowerCaseMap() redundant
Delete toLowerCaseMap() and revert RxGatewayStoreModel callers to toMap()
Also fixes direct-connect mode (ResponseUtils, HttpClientUtils) which still used toMap() with original casing (latent Fabric-class bug)

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

Option A: Skip redundant toLowerCase() for HTTP/2 headers - Add keysAlreadyLowerCase flag to Cosmos HttpHeaders - When true, set() skips the toLowerCase() call on header names - Detect HTTP/2 in ReactorNettyHttpResponse.headers() via version check - Saves ~15-25 toLowerCase calls + String allocations per response - HTTP/2 headers are guaranteed lowercase per RFC 7540 §8.1.2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

xinlian12 · 2026-03-19T05:04:30Z

@sdkReviewAgent

xinlian12 · 2026-03-19T05:31:29Z

sdkReviewAgent | Status: ⏳ Queued

Review requested by @xinlian12. I'll start shortly.

xinlian12 · 2026-03-19T05:31:30Z

sdkReviewAgent | Status: 🔍 Reviewing

I'm reviewing this PR now. I'll post my findings as comments when done.

xinlian12 · 2026-03-19T05:46:15Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/HttpHeaders.java

+    public HttpHeaders(int size, boolean isHttp2Headers) {
+        this.headers = new HashMap<>(size);
+        this.isHttp2Headers = isHttp2Headers;
    }



Recommendation
isHttp2Headers optimization is not active — no production caller

This constructor and the conditional toLowerCase() skip in set() are added, but ReactorNettyClient.java:412 — the sole production site where HTTP response headers are constructed — still calls:
new HttpHeaders(reactorNettyResponse.responseHeaders().size())
which defaults isHttp2Headers to false. Grepping for the new constructor across sdk/cosmos/ returns zero production matches.

This means the PR's claimed saving of ~15-25 toLowerCase() calls per HTTP/2 response is not realized. The flag and constructor are dead code.

Suggestion: Either wire the flag into ReactorNettyHttpResponse.headers() in this PR (e.g. checking reactorNettyResponse.version() for HTTP/2), or defer this infrastructure to the PR that includes the wiring. Shipping dead infrastructure creates maintenance burden without delivering value.

_{Powered by AI}

xinlian12 · 2026-03-19T18:07:44Z

@sdkReviewBot

xinlian12 · 2026-03-19T18:08:39Z

@sdkReviewAgent

xinlian12 · 2026-03-19T18:09:25Z

sdkReviewAgent | Status: ⏳ Queued

Review requested by @xinlian12. I'll start shortly.

xinlian12 · 2026-03-19T18:09:26Z

sdkReviewAgent | Status: 🔍 Reviewing

I'm reviewing this PR now. I'll post my findings as comments when done.

xinlian12 · 2026-03-19T18:24:12Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/HttpHeaders.java

 */
 public class HttpHeaders implements Iterable<HttpHeader>, JsonSerializable {
    private Map<String, HttpHeader> headers;
+    private final boolean isHttp2Headers;


Observation
PR Review Summary

What this PR does: Two changes: (1) refactors Http2ResponseHeaderCleanerHandler from O(n) forEach to O(1) AsciiString lookup — clean and correct; (2) adds this isHttp2Headers flag to skip toLowerCase() in set() for HTTP/2 responses.

Handler changes are likely already merged from PR #48455 (P1). The branch may need rebasing — after rebase, those handler changes should disappear from the diff.

Option C is described but not implemented. The PR description mentions "Eliminate toLowerCaseMap() as redundant", "revert RxGatewayStoreModel to toMap()", and "fix direct-connect mode (ResponseUtils, HttpClientUtils)". None of these changes are in the diff. Consider updating the description to reflect only the actual changes.

Architecture note: This flag introduces transport-protocol awareness into HttpHeaders, which is otherwise a generic data container. The existing pattern isolates HTTP/2 concerns in pipeline handlers, not in data structures that flow through business logic. If the flag stays, document that it must only be set by the reactor-netty response path.

_{Powered by AI}

xinlian12 · 2026-03-19T18:24:23Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/HttpHeaders.java

     */
    public HttpHeaders set(String name, String value) {
-        final String headerKey = name.toLowerCase(Locale.ROOT);
+        final String headerKey = isHttp2Headers ? name : name.toLowerCase(Locale.ROOT);


Recommendation
set() / getHeader() asymmetry creates a latent footgun

When isHttp2Headers=true, set() stores the map key without lowercasing, but getHeader() (line 102) always lowercases the lookup key:

// In set(): final String headerKey = isHttp2Headers ? name : name.toLowerCase(Locale.ROOT); // In getHeader(): final String headerKey = headerName.toLowerCase(Locale.ROOT); // always lowercases

If anyone calls set("Content-Type", value) on an isHttp2Headers=true instance, the header is stored under key "Content-Type" but value("Content-Type") looks up "content-type" → silent miss. The header appears set but is unretrievable.

This is safe today because HTTP/2 guarantees lowercase and the constructor has no production callers. But when it gets wired up, a future developer could easily misuse it. Consider adding a defensive assertion:

assert !isHttp2Headers || name.equals(name.toLowerCase(Locale.ROOT)) : "isHttp2Headers requires lowercase names, got: " + name;

Or, make getHeader() also honor the flag to keep the two paths symmetric.

_{Powered by AI}

xinlian12 · 2026-03-19T18:24:37Z

.../azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/HttpHeadersTests.java

+    public void keysAlreadyLowerCaseSkipsNormalization() {
+        String headerName = "etag";
+        String headerValue = "456";
+
+        // Simulates HTTP/2 where header names are already lowercase
+        HttpHeaders headers = new HttpHeaders(4, true);
+        headers.set(headerName, headerValue);
+
+        Map<String, String> map = headers.toMap();
+        assertThat(map.get(headerName)).isEqualTo(headerValue);


Suggestion
Test doesn't prove the optimization is exercised, and misses value() coverage

Two gaps:

The test uses "etag" (already lowercase), so "etag".toLowerCase() == "etag" — the result is identical whether or not the flag is true. Changing new HttpHeaders(4, true) to new HttpHeaders(4) would still pass. The test proves correctness but not that the optimization path is actually different.

value() lookup is not tested with isHttp2Headers=true. The primary consumer API (value() / getHeader()) is uncovered for this flag. Add assertions like:

assertThat(headers.value("etag")).isEqualTo(headerValue); assertThat(headers.value("ETag")).isEqualTo(headerValue); // verifies getHeader() lowercases lookup

This would also serve as a regression guard against the set()/getHeader() asymmetry noted in HttpHeaders.java.

_{Powered by AI}

Imports the shared review pipeline from xinlian12/sdk-auto-pr-review. Triggers on PR open/push/ready_for_review. Posts inline review comments with severity tags and AI disclaimer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add sdkReviewAgent agentic workflow for automated PR review

xinlian12 · 2026-03-19T20:24:03Z

@sdkReviewAgent

xinlian12 · 2026-03-19T20:24:27Z

sdkReviewAgent | Status: ⏳ Queued

Review requested by @xinlian12. I'll start shortly.

xinlian12 · 2026-03-19T20:24:29Z

sdkReviewAgent | Status: 🔍 Reviewing

I'm reviewing this PR now. I'll post my findings as comments when done.

github-actions bot added the Cosmos label Mar 18, 2026

xinlian12 force-pushed the perf/p1-fix-http2-toLowerCase branch from beb48da to 8ba91eb Compare March 18, 2026 18:19

xinlian12 force-pushed the perf/p1-fix-http2-toLowerCase branch from 8ba91eb to 63c1521 Compare March 18, 2026 19:02

xinlian12 commented Mar 19, 2026

View reviewed changes

Annie Liang and others added 4 commits March 19, 2026 13:16

Merge pull request #22 from xinlian12/feature/sdk-review-agent

097e47c

Add sdkReviewAgent agentic workflow for automated PR review

Merge branch 'main' of https://github.com/xinlian12/azure-sdk-for-java

284cdb8

merge from main and resolve conflicts

2be1f2f

Annie Liang added 3 commits March 19, 2026 13:39

trigger run

42768ba

trigger run

1ea9ea8

trigger run

272ee51

Conversation

xinlian12 commented Mar 18, 2026

Description

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

xinlian12 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

xinlian12 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

xinlian12 commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant