From 294d30ff35fa0fbe8bbe4aa4aadb86a5469bbc01 Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 13:03:51 -0400 Subject: [PATCH 1/6] perf: install Http2PingCloseRewrapHandler in doOnConnected, not doOnRequest The previous .doOnRequest() chained onto the HTTP/2 HttpClient builder wrapped every outgoing request's Mono in a Reactor operator. That paid per-request costs even when the rewrap handler was already installed: * MonoPeekTerminal wrap + per-request subscriber allocation (InternalMonoOperator.subscribe +2.60% in alloc profile) * per-request BiConsumer lambda invocation (Http2Pool\$\...run +1.16% in alloc profile) * per-request channel pipeline String-key walk via pipeline.get(...) Long-run async-profiler diff vs v4.80.0 on D2 attributed the PR's -7.6% QPS / +14.5% mean / +166% tail-max regression primarily to a +10.2% allocation rate -- with the operators above dominating the delta. Move the install into the existing .doOnConnected() block, which already runs once per HTTP/2 child stream (the customHeaderCleaner install right above it relies on the same contract). The rewrap handler is @Sharable, so installing once per stream is correct and amortizes the install + pipeline-walk away from the hot path. The ch.parent() != null guard is kept so the install only targets child streams, never the parent TCP channel (which uses Http2ParentChannelExceptionHandler instead). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../http/ReactorNettyClient.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java index e95989c5aa3c..3e6008405869 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java @@ -223,8 +223,7 @@ private void configureChannelPipelineHandlers() { // Duplicate handler name is the only possible cause. } } - })) - .doOnRequest((req, conn) -> { + // Install a @Sharable head-of-pipeline rewrap handler on each H2 // child-stream pipeline. When Http2PingHandler closes the parent // (TCP) channel after consecutive PING-ACK timeouts or PING-send @@ -237,21 +236,31 @@ private void configureChannelPipelineHandlers() { // stack maps that to GATEWAY_HTTP2_PING_TIMEOUT_CHANNEL_CLOSED so // ClientRetryPolicy can suppress region mark-down. // + // doOnConnected fires once per child stream channel (the existing + // customHeaderCleaner install above relies on the same contract), so + // this install is amortized to one-time per stream rather than + // per-request -- avoids the operator-wrap + per-request pipeline + // walk that the prior .doOnRequest()-based install paid on every + // HTTP call. + // // Gate on ch.parent() != null so this only runs on H2 child streams - // (H1.1 connections have null parent and never need the rewrap). - Channel ch = conn.channel(); - if (ch.parent() != null && ch.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { + // (the parent TCP channel uses Http2ParentChannelExceptionHandler + // installed above; H1.1 connections never enter this branch since + // the entire enclosing block is guarded by isH2Enabled). + Channel ch = connection.channel(); + if (ch.parent() != null + && channelPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { try { - ch.pipeline().addFirst( + channelPipeline.addFirst( Http2PingCloseRewrapHandler.HANDLER_NAME, Http2PingCloseRewrapHandler.INSTANCE); } catch (IllegalArgumentException ignored) { // TOCTOU race: between the get()==null check above and addFirst(), - // a concurrent doOnRequest may have installed the handler. + // a concurrent doOnConnected may have installed the handler. // Duplicate handler name is the only possible cause. } } - }); + })); } } From dd240560cb143a5486c2048b1c966a3ca861a9e6 Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 15:20:38 -0400 Subject: [PATCH 2/6] perf: gate Http2PingCloseRewrapHandler install on isPingHealthEffectivelyEnabled Follow-up to the doOnConnected install move (previous commit on this PR). The rewrap handler translates Http2MultiplexHandler's per-child-stream channelInactive into a typed Http2PingTimeoutChannelClosedException when the PING sender closes the parent TCP channel after consecutive PING-ACK timeouts. If the PING sender is disabled there is no such signal to translate, so the install adds per-child-stream pipeline-add cost and an extra pipeline hop on every inbound frame for zero behavioral benefit. Empirical motivation: long-run perf matrix on a 2 vCPU D2s_v5 VM (point-read workload, Gateway+H2, ~98% CPU steady state, baseline = v4.80.0): baseline (v4.80.0) : 9010 QPS ping-prefix (PR before doOnConnected fix) : 8301 QPS (-7.9%) ping-postfix (doOnConnected install only) : 8170 QPS (-9.3%) ping-off (postfix + HTTP2_PING_HEALTH=false): 8102 QPS (-10.1%) Toggling the documented kill switch did not recover baseline because the rewrap install was unconditional -- the kill switch only suppressed the PING sender, not the per-child-stream rewrap install. async-profiler CPU diffs ping-off-vs-baseline and ping-off-vs-postfix were essentially identical, confirming the rewrap install dominates the residual cost. Consolidating both PING-sender install and rewrap install under Http2PingHandler.isPingHealthEffectivelyEnabled makes HTTP2_PING_HEALTH_ENABLED=false a true revert-to-baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- sdk/cosmos/azure-cosmos/CHANGELOG.md | 1 + .../implementation/http/ReactorNettyClient.java | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/sdk/cosmos/azure-cosmos/CHANGELOG.md b/sdk/cosmos/azure-cosmos/CHANGELOG.md index 4dfa21796c50..6a152d5b16bd 100644 --- a/sdk/cosmos/azure-cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure-cosmos/CHANGELOG.md @@ -13,6 +13,7 @@ * Fixed region name normalization for preferred and excluded regions — non-canonical inputs (e.g., `"westus3"`, `"WEST US 3"`) are now mapped to the canonical form. Also fixed a case-sensitive exclude-region check in PPCB reevaluate logic. - See [PR 49090](https://github.com/Azure/azure-sdk-for-java/pull/49090) * Fixed `UnsupportedOperationException` when using `readManyByPartitionKeys` for empty pages. - See [PR 49311](https://github.com/Azure/azure-sdk-for-java/pull/49311) * Fixed silent drift in `CosmosChangeFeedRequestOptions` when resuming from a continuation token via `byPage(savedContinuation)`. Previously only `maxPrefetchPageCount` and `throughputControlGroupName` were inherited onto the rebuilt impl; `endLSN`, `customSerializer`, `excludeRegions`, `readConsistencyStrategy`, `completeAfterAllCurrentChangesRetrieved`, and other caller-supplied configuration were silently dropped. All non-token-encoded fields are now propagated. - See [PR 49276](https://github.com/Azure/azure-sdk-for-java/pull/49276) +* Fixed two HTTP/2 PING-handler perf regressions (introduced in [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095)): (1) `Http2PingCloseRewrapHandler` install moved from `.doOnRequest()` (per-HTTP-call operator wrap + per-request pipeline walk) into `.doOnConnected()` so it is amortised to one install per H2 child stream; (2) the rewrap install is now gated on `Http2PingHandler.isPingHealthEffectivelyEnabled` so disabling PING-health (`COSMOS.HTTP2_PING_HEALTH_ENABLED=false`) is a true revert-to-baseline rather than only suppressing the PING sender. #### Other Changes * Added HTTP/2 PING keepalive (default ON) for Gateway service endpoints to detect silently-broken connections. - See [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java index 3e6008405869..47508702f7c0 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java @@ -247,8 +247,20 @@ private void configureChannelPipelineHandlers() { // (the parent TCP channel uses Http2ParentChannelExceptionHandler // installed above; H1.1 connections never enter this branch since // the entire enclosing block is guarded by isH2Enabled). + // + // Also gate on Http2PingHandler.isPingHealthEffectivelyEnabled: the + // rewrap handler exists only to translate channelInactive into a typed + // Http2PingTimeoutChannelClosedException when the PING sender closes + // the parent channel after consecutive PING-ACK timeouts. If the PING + // sender is disabled (via COSMOS.HTTP2_PING_HEALTH_ENABLED=false or the + // user-agent feature flag), the rewrap handler has no signal to translate + // -- installing it would add per-child-stream pipeline-add cost and an + // extra pipeline hop for every inbound frame without any behavioral + // benefit. The gate consolidates the install lifecycle with the rest of + // the PING-health feature so the kill-switch is a true revert-to-baseline. Channel ch = connection.channel(); if (ch.parent() != null + && Http2PingHandler.isPingHealthEffectivelyEnabled(http2Cfg) && channelPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { try { channelPipeline.addFirst( From 2e008997890faf873fbdb55e67a0a9e0005114ed Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 17:21:47 -0400 Subject: [PATCH 3/6] fix(cosmos): install HTTP/2 PING rewrap handler via observe(STREAM_CONFIGURED) The rewrap handler install lived inside .doOnConnected(...), which reactor-netty fires only on the H2 parent TCP channel (State.CONFIGURED, parent==null). The install was gated on ch.parent()!=null, so it never ran on the parent and never reached a child stream -- the handler was installed 0 times and PING-timeout parent closes surfaced as bare PrematureCloseException instead of the typed Http2PingTimeoutChannelClosedException. Move the install to .observe((connection, state) -> ...) keyed on HttpClientState.STREAM_CONFIGURED, the per-child-stream lifecycle event. The handler is now added at the head of each H2 child-stream pipeline, where it intercepts channelInactive ahead of HttpClientOperations and rewraps the close into the typed exception so ClientRetryPolicy can suppress region mark-down. Keeps the PING-health gate, ch.parent()!=null defensive check, get(HANDLER_NAME)==null guard, and TOCTOU try/catch. Also fixes stale doc-comments referencing the old install hook. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- sdk/cosmos/azure-cosmos/CHANGELOG.md | 2 +- .../http/Http2PingCloseRewrapHandler.java | 6 +- .../implementation/http/Http2PingHandler.java | 3 +- .../http/ReactorNettyClient.java | 93 ++++++++++--------- 4 files changed, 56 insertions(+), 48 deletions(-) diff --git a/sdk/cosmos/azure-cosmos/CHANGELOG.md b/sdk/cosmos/azure-cosmos/CHANGELOG.md index 6a152d5b16bd..3382ffc901d5 100644 --- a/sdk/cosmos/azure-cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure-cosmos/CHANGELOG.md @@ -13,7 +13,7 @@ * Fixed region name normalization for preferred and excluded regions — non-canonical inputs (e.g., `"westus3"`, `"WEST US 3"`) are now mapped to the canonical form. Also fixed a case-sensitive exclude-region check in PPCB reevaluate logic. - See [PR 49090](https://github.com/Azure/azure-sdk-for-java/pull/49090) * Fixed `UnsupportedOperationException` when using `readManyByPartitionKeys` for empty pages. - See [PR 49311](https://github.com/Azure/azure-sdk-for-java/pull/49311) * Fixed silent drift in `CosmosChangeFeedRequestOptions` when resuming from a continuation token via `byPage(savedContinuation)`. Previously only `maxPrefetchPageCount` and `throughputControlGroupName` were inherited onto the rebuilt impl; `endLSN`, `customSerializer`, `excludeRegions`, `readConsistencyStrategy`, `completeAfterAllCurrentChangesRetrieved`, and other caller-supplied configuration were silently dropped. All non-token-encoded fields are now propagated. - See [PR 49276](https://github.com/Azure/azure-sdk-for-java/pull/49276) -* Fixed two HTTP/2 PING-handler perf regressions (introduced in [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095)): (1) `Http2PingCloseRewrapHandler` install moved from `.doOnRequest()` (per-HTTP-call operator wrap + per-request pipeline walk) into `.doOnConnected()` so it is amortised to one install per H2 child stream; (2) the rewrap install is now gated on `Http2PingHandler.isPingHealthEffectivelyEnabled` so disabling PING-health (`COSMOS.HTTP2_PING_HEALTH_ENABLED=false`) is a true revert-to-baseline rather than only suppressing the PING sender. +* Fixed the HTTP/2 PING-timeout rewrap handler (`Http2PingCloseRewrapHandler`, introduced in [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095)) never being installed. It is now added at the head of each HTTP/2 child-stream pipeline via reactor-netty's `.observe(...)` hook on the `STREAM_CONFIGURED` lifecycle event. The prior `.doOnConnected()` install fired only on the parent TCP channel and never reached child streams, so PING-timeout parent closes surfaced as a bare `PrematureCloseException` instead of the typed `Http2PingTimeoutChannelClosedException` (preventing `ClientRetryPolicy` from suppressing region mark-down). The install is gated on `Http2PingHandler.isPingHealthEffectivelyEnabled` so disabling PING-health (`COSMOS.HTTP2_PING_HEALTH_ENABLED=false`) is a true revert-to-baseline. - See [PR 49403](https://github.com/Azure/azure-sdk-for-java/pull/49403) #### Other Changes * Added HTTP/2 PING keepalive (default ON) for Gateway service endpoints to detect silently-broken connections. - See [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java index 02f6cd67dbcf..a529d4c22088 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java @@ -12,7 +12,8 @@ * driven by {@link Http2PingHandler} into a typed {@link Http2PingTimeoutChannelClosedException}. *

* Installed at the head of each H2 child-stream pipeline by - * {@code ReactorNettyClient}'s {@code .doOnRequest(...)} hook. When the parent (TCP) + * {@code ReactorNettyClient}'s {@code .observe(...)} hook on {@code STREAM_CONFIGURED} + * (the per-child-stream lifecycle event). When the parent (TCP) * channel is closed by {@link Http2PingHandler} after consecutive PING-ACK timeouts * or consecutive PING-send failures, the H2 multiplex codec propagates * {@code channelInactive} to every child stream. @@ -29,7 +30,8 @@ * JVM-wide {@link #INSTANCE} is reused across all H2 child streams. *

* For non-H2 channels (parent is {@code null}) this handler is never installed; the - * install site in {@code ReactorNettyClient} gates on {@code ch.parent() != null}. + * install site in {@code ReactorNettyClient} only runs at {@code STREAM_CONFIGURED} + * (H2 child streams) and additionally gates on {@code ch.parent() != null}. */ @ChannelHandler.Sharable final class Http2PingCloseRewrapHandler extends ChannelInboundHandlerAdapter { diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingHandler.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingHandler.java index 82782d14c71b..a20ed23b7715 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingHandler.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingHandler.java @@ -52,7 +52,8 @@ public class Http2PingHandler extends ChannelDuplexHandler { *

* Consumed by {@link Http2PingCloseRewrapHandler}, a {@code @Sharable} handler * installed at the head of each H2 child-stream pipeline by {@code - * ReactorNettyClient.doOnRequest(...)}. When the parent channel closes with this + * ReactorNettyClient}'s {@code .observe(...)} hook on {@code STREAM_CONFIGURED}. When + * the parent channel closes with this * attribute set, the rewrap handler fires {@code exceptionCaught} with a typed * {@link Http2PingTimeoutChannelClosedException} so the in-flight request's * response {@code Mono} fails with the typed exception (instead of a bare diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java index 47508702f7c0..7e24769863f8 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java @@ -224,55 +224,60 @@ private void configureChannelPipelineHandlers() { } } + })) + .observe((connection, state) -> { // Install a @Sharable head-of-pipeline rewrap handler on each H2 - // child-stream pipeline. When Http2PingHandler closes the parent - // (TCP) channel after consecutive PING-ACK timeouts or PING-send - // failures, the H2 multiplex codec propagates channelInactive to - // every child stream; the rewrap handler intercepts that and fires - // exceptionCaught with a typed - // Http2PingTimeoutChannelClosedException so reactor-netty's - // HttpClientOperations fails the response Mono with the typed - // exception (instead of bare PrematureCloseException). The rest of the - // stack maps that to GATEWAY_HTTP2_PING_TIMEOUT_CHANNEL_CLOSED so - // ClientRetryPolicy can suppress region mark-down. + // child-stream pipeline. STREAM_CONFIGURED is the per-child-stream + // lifecycle event reactor-netty fires once when a stream is opened, so + // connection.channel() here is the child stream (its parent is the + // parent TCP channel). doOnConnected cannot be used for this install: + // it fires only on the parent TCP channel (State.CONFIGURED) and never + // on a child stream, so a ch.parent() != null gate inside doOnConnected + // would never be satisfied and the handler would never install. // - // doOnConnected fires once per child stream channel (the existing - // customHeaderCleaner install above relies on the same contract), so - // this install is amortized to one-time per stream rather than - // per-request -- avoids the operator-wrap + per-request pipeline - // walk that the prior .doOnRequest()-based install paid on every - // HTTP call. + // The rewrap MUST live on the child stream pipeline, not the parent. + // When Http2PingHandler closes the parent (TCP) channel after + // consecutive PING-ACK timeouts or PING-send failures, the H2 multiplex + // codec propagates channelInactive to each child stream independently. + // Child streams are separate Netty Channels, so that close does not + // surface on the parent pipeline -- reactor-netty's per-stream + // HttpClientOperations turns the child channelInactive into a bare + // PrematureCloseException. This head-of-child-pipeline handler + // intercepts channelInactive first and fires exceptionCaught with a + // typed Http2PingTimeoutChannelClosedException before HttpClientOperations + // observes the close, so the response Mono fails with the typed + // exception. The rest of the stack maps that to + // GATEWAY_HTTP2_PING_TIMEOUT_CHANNEL_CLOSED so ClientRetryPolicy can + // suppress region mark-down. // - // Gate on ch.parent() != null so this only runs on H2 child streams - // (the parent TCP channel uses Http2ParentChannelExceptionHandler - // installed above; H1.1 connections never enter this branch since - // the entire enclosing block is guarded by isH2Enabled). - // - // Also gate on Http2PingHandler.isPingHealthEffectivelyEnabled: the - // rewrap handler exists only to translate channelInactive into a typed - // Http2PingTimeoutChannelClosedException when the PING sender closes - // the parent channel after consecutive PING-ACK timeouts. If the PING - // sender is disabled (via COSMOS.HTTP2_PING_HEALTH_ENABLED=false or the - // user-agent feature flag), the rewrap handler has no signal to translate - // -- installing it would add per-child-stream pipeline-add cost and an - // extra pipeline hop for every inbound frame without any behavioral - // benefit. The gate consolidates the install lifecycle with the rest of - // the PING-health feature so the kill-switch is a true revert-to-baseline. - Channel ch = connection.channel(); - if (ch.parent() != null - && Http2PingHandler.isPingHealthEffectivelyEnabled(http2Cfg) - && channelPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { - try { - channelPipeline.addFirst( - Http2PingCloseRewrapHandler.HANDLER_NAME, - Http2PingCloseRewrapHandler.INSTANCE); - } catch (IllegalArgumentException ignored) { - // TOCTOU race: between the get()==null check above and addFirst(), - // a concurrent doOnConnected may have installed the handler. - // Duplicate handler name is the only possible cause. + // Gate on Http2PingHandler.isPingHealthEffectivelyEnabled: the rewrap + // handler only has a signal to translate while the PING sender is active. + // If PING-health is disabled (via COSMOS.HTTP2_PING_HEALTH_ENABLED=false + // or the user-agent feature flag), skip the install so the kill-switch is + // a true revert-to-baseline with no extra per-stream pipeline hop. This + // gate is evaluated once per stream at install time; toggling the + // kill-switch on at runtime only affects streams configured afterwards. + // Streams are single-use, so behavior converges within one stream lifetime. + if (state == HttpClientState.STREAM_CONFIGURED + && Http2PingHandler.isPingHealthEffectivelyEnabled(http2Cfg)) { + Channel ch = connection.channel(); + ChannelPipeline childPipeline = ch.pipeline(); + // STREAM_CONFIGURED implies a child stream, so ch.parent() is the + // parent TCP channel; the null-check is defensive. + if (ch.parent() != null + && childPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { + try { + childPipeline.addFirst( + Http2PingCloseRewrapHandler.HANDLER_NAME, + Http2PingCloseRewrapHandler.INSTANCE); + } catch (IllegalArgumentException ignored) { + // Benign duplicate install: another install path may have + // added the handler between the get()==null check above and + // addFirst(). A duplicate handler name is the only cause. + } } } - })); + }); } } From de9a6c3d16ea333ece89e144dfb55cd153d43366 Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 17:32:15 -0400 Subject: [PATCH 4/6] docs(cosmos): tighten Http2PingCloseRewrapHandler class-doc wording The handler is a single @Sharable instance installed per H2 child stream, not a per-request object. Reword the class-doc summary from 'Per-request' to 'Per-child-stream' for consistency with the rest of the doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../cosmos/implementation/http/Http2PingCloseRewrapHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java index a529d4c22088..7e6b0dd511c4 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapHandler.java @@ -8,7 +8,7 @@ import io.netty.channel.ChannelInboundHandlerAdapter; /** - * Per-request HTTP/2 child-stream handler that translates a parent-TCP-channel close + * Per-child-stream HTTP/2 handler that translates a parent-TCP-channel close * driven by {@link Http2PingHandler} into a typed {@link Http2PingTimeoutChannelClosedException}. *

* Installed at the head of each H2 child-stream pipeline by From ec9d1a1fa40afb3479c40b84efc4f32b51964419 Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 18:26:49 -0400 Subject: [PATCH 5/6] Add CI unit test for HTTP/2 PING close-rewrap install gate Extract the inline .observe() install decision into a package-private static installHttp2PingCloseRewrapHandlerIfNeeded(...) so the per-child-stream gate is unit-testable without a live HTTP/2 connection. Add a TestNG unit test (group "unit") that drives the real method via an EmbeddedChannel and proves the disablement path (kill-switch off, non-positive PING interval, HTTP/2 disabled, non-STREAM_CONFIGURED state, parent-less channel) is a true revert-to-baseline that installs nothing -- plus positive install, head-of-pipeline position, idempotency, and state-before-predicate short-circuit ordering. Behavior-preserving refactor; no user-facing change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../http/Http2PingCloseRewrapInstallTest.java | 256 ++++++++++++++++++ .../http/ReactorNettyClient.java | 143 ++++++---- 2 files changed, 346 insertions(+), 53 deletions(-) create mode 100644 sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapInstallTest.java diff --git a/sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapInstallTest.java b/sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapInstallTest.java new file mode 100644 index 000000000000..a09bc5db0a22 --- /dev/null +++ b/sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/Http2PingCloseRewrapInstallTest.java @@ -0,0 +1,256 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package com.azure.cosmos.implementation.http; + +import com.azure.cosmos.Http2ConnectionConfig; +import io.netty.channel.Channel; +import io.netty.channel.embedded.EmbeddedChannel; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; +import reactor.netty.http.client.HttpClientState; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Unit tests for {@link ReactorNettyClient#installHttp2PingCloseRewrapHandlerIfNeeded} -- + * the per-child-stream install gate for {@link Http2PingCloseRewrapHandler}. + *

+ * In production the handler is installed from reactor-netty's {@code .observe(...)} hook + * only when ALL of the following hold: + *

    + *
  • the connection-lifecycle state is {@link HttpClientState#STREAM_CONFIGURED} + * (a child stream was just opened);
    • + *
  • PING-health is effectively enabled -- kill-switch + * {@code COSMOS.HTTP2_PING_HEALTH_ENABLED} on, a positive PING interval, and HTTP/2 + * enabled for the client;
    • + *
  • the channel is a child stream ({@code parent() != null}).
    • + *
+ * These tests drive the real production method with an {@link EmbeddedChannel} so the + * disablement path -- flipping the kill-switch (or any gate input) off must be a true + * revert-to-baseline that installs nothing on the child pipeline -- is guarded in CI + * without needing a live HTTP/2 server. + */ +public class Http2PingCloseRewrapInstallTest { + + private static final String PING_HEALTH_ENABLED = "COSMOS.HTTP2_PING_HEALTH_ENABLED"; + private static final String PING_INTERVAL_SECONDS = "COSMOS.HTTP2_PING_INTERVAL_IN_SECONDS"; + + private String priorPingHealthEnabled; + private String priorPingIntervalSeconds; + + @BeforeMethod(groups = "unit") + public void before_Method() { + // Snapshot the two PING system properties the gate reads so each test sets an + // explicit, isolated state and never leaks into sibling tests or CI defaults. + this.priorPingHealthEnabled = System.getProperty(PING_HEALTH_ENABLED); + this.priorPingIntervalSeconds = System.getProperty(PING_INTERVAL_SECONDS); + } + + @AfterMethod(groups = "unit", alwaysRun = true) + public void after_Method() { + restore(PING_HEALTH_ENABLED, this.priorPingHealthEnabled); + restore(PING_INTERVAL_SECONDS, this.priorPingIntervalSeconds); + } + + @Test(groups = "unit") + public void installsHandler_whenStreamConfiguredAndPingHealthEnabled() { + enablePingHealth(); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + + // Presence AND head-of-pipeline position: the rewrap handler must see + // channelInactive before reactor-netty's stream operations handler, so a + // regression from addFirst(...) to addLast(...) must fail this test. + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNotNull(); + assertThat(child.pipeline().first()).isSameAs(Http2PingCloseRewrapHandler.INSTANCE); + assertThat(child.pipeline().names().get(0)).isEqualTo(Http2PingCloseRewrapHandler.HANDLER_NAME); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_whenKillSwitchOff() { + // Disablement path: COSMOS.HTTP2_PING_HEALTH_ENABLED=false must remove the work, + // not just its effect -- nothing is added to the child pipeline. + System.setProperty(PING_HEALTH_ENABLED, "false"); + System.setProperty(PING_INTERVAL_SECONDS, "1"); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_whenPingIntervalNonPositive() { + // A non-positive PING interval disables the PING sender, so there is no + // PING-timeout close signal to rewrap -> install is skipped. + System.setProperty(PING_HEALTH_ENABLED, "true"); + System.setProperty(PING_INTERVAL_SECONDS, "0"); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_whenHttp2DisabledForClient() { + // PING-health globally on, but HTTP/2 disabled on this client's config -> + // isPingHealthEffectivelyEnabled is false -> no install. + enablePingHealth(); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Disabled()); + + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_whenStateNotStreamConfigured() { + // Even with PING-health fully enabled, non-STREAM_CONFIGURED lifecycle events + // (e.g. the parent-channel CONFIGURED state) must not install the child handler. + enablePingHealth(); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.CONFIGURED, http2Enabled()); + + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_andDoesNotEvaluatePredicate_whenStateNotStreamConfigured() { + // Guards the short-circuit order: the state check must run BEFORE the PING-health + // predicate. A null http2Cfg would NPE inside isPingHealthEffectivelyEnabled (the + // bridge accessor dereferences it), so a non-STREAM_CONFIGURED state must return + // early without touching the predicate -- proving the cheap state gate stays first + // and off the hot path. + enablePingHealth(); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.CONFIGURED, null); + + assertThat(child.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + releaseAll(child); + } + } + + @Test(groups = "unit") + public void skipsInstall_whenChannelHasNoParent() { + // Defensive parent() != null guard: a parent-less channel (not a real H2 child + // stream) must not get the rewrap handler even at STREAM_CONFIGURED. + enablePingHealth(); + EmbeddedChannel parentless = new EmbeddedChannel(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + parentless, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + + assertThat(parentless.pipeline().get(Http2PingCloseRewrapHandler.HANDLER_NAME)).isNull(); + } finally { + parentless.finishAndReleaseAll(); + } + } + + @Test(groups = "unit") + public void install_isIdempotent() { + // The @Sharable handler must be installed at most once per child pipeline even if + // the observe hook fires STREAM_CONFIGURED more than once for the same stream. + enablePingHealth(); + ChildChannel child = newChildStream(); + try { + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + ReactorNettyClient.installHttp2PingCloseRewrapHandlerIfNeeded( + child, HttpClientState.STREAM_CONFIGURED, http2Enabled()); + + long count = child.pipeline().toMap().values().stream() + .filter(h -> h instanceof Http2PingCloseRewrapHandler) + .count(); + assertThat(count).isEqualTo(1); + } finally { + releaseAll(child); + } + } + + private static void enablePingHealth() { + System.setProperty(PING_HEALTH_ENABLED, "true"); + System.setProperty(PING_INTERVAL_SECONDS, "1"); + } + + // Explicit enabled/disabled flags so the gate's HTTP/2 condition does not depend on + // the ambient COSMOS.HTTP2_ENABLED system property. Constructing the config also runs + // Http2ConnectionConfig's static initializer, registering the bridge accessor the gate + // reads. + private static Http2ConnectionConfig http2Enabled() { + return new Http2ConnectionConfig().setEnabled(true); + } + + private static Http2ConnectionConfig http2Disabled() { + return new Http2ConnectionConfig().setEnabled(false); + } + + private static ChildChannel newChildStream() { + ChildChannel child = new ChildChannel(); + child.setParentChannel(new EmbeddedChannel()); + return child; + } + + private static void restore(String key, String prior) { + if (prior == null) { + System.clearProperty(key); + } else { + System.setProperty(key, prior); + } + } + + private static void releaseAll(ChildChannel child) { + Channel parent = child.parent(); + child.finishAndReleaseAll(); + if (parent instanceof EmbeddedChannel) { + ((EmbeddedChannel) parent).finishAndReleaseAll(); + } + } + + /** + * An {@link EmbeddedChannel} whose {@link #parent()} can be set after construction, + * mimicking an HTTP/2 child stream whose parent is the TCP connection channel. The + * parent field is assigned post-construction (never read during the superclass + * constructor), which avoids the captured-variable-before-super pitfall of an + * anonymous subclass. + */ + private static final class ChildChannel extends EmbeddedChannel { + private Channel parentChannel; + + @Override + public Channel parent() { + return this.parentChannel; + } + + void setParentChannel(Channel parent) { + this.parentChannel = parent; + } + } +} diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java index 7e24769863f8..bdd6e68453b2 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java @@ -225,59 +225,96 @@ private void configureChannelPipelineHandlers() { } })) - .observe((connection, state) -> { - // Install a @Sharable head-of-pipeline rewrap handler on each H2 - // child-stream pipeline. STREAM_CONFIGURED is the per-child-stream - // lifecycle event reactor-netty fires once when a stream is opened, so - // connection.channel() here is the child stream (its parent is the - // parent TCP channel). doOnConnected cannot be used for this install: - // it fires only on the parent TCP channel (State.CONFIGURED) and never - // on a child stream, so a ch.parent() != null gate inside doOnConnected - // would never be satisfied and the handler would never install. - // - // The rewrap MUST live on the child stream pipeline, not the parent. - // When Http2PingHandler closes the parent (TCP) channel after - // consecutive PING-ACK timeouts or PING-send failures, the H2 multiplex - // codec propagates channelInactive to each child stream independently. - // Child streams are separate Netty Channels, so that close does not - // surface on the parent pipeline -- reactor-netty's per-stream - // HttpClientOperations turns the child channelInactive into a bare - // PrematureCloseException. This head-of-child-pipeline handler - // intercepts channelInactive first and fires exceptionCaught with a - // typed Http2PingTimeoutChannelClosedException before HttpClientOperations - // observes the close, so the response Mono fails with the typed - // exception. The rest of the stack maps that to - // GATEWAY_HTTP2_PING_TIMEOUT_CHANNEL_CLOSED so ClientRetryPolicy can - // suppress region mark-down. - // - // Gate on Http2PingHandler.isPingHealthEffectivelyEnabled: the rewrap - // handler only has a signal to translate while the PING sender is active. - // If PING-health is disabled (via COSMOS.HTTP2_PING_HEALTH_ENABLED=false - // or the user-agent feature flag), skip the install so the kill-switch is - // a true revert-to-baseline with no extra per-stream pipeline hop. This - // gate is evaluated once per stream at install time; toggling the - // kill-switch on at runtime only affects streams configured afterwards. - // Streams are single-use, so behavior converges within one stream lifetime. - if (state == HttpClientState.STREAM_CONFIGURED - && Http2PingHandler.isPingHealthEffectivelyEnabled(http2Cfg)) { - Channel ch = connection.channel(); - ChannelPipeline childPipeline = ch.pipeline(); - // STREAM_CONFIGURED implies a child stream, so ch.parent() is the - // parent TCP channel; the null-check is defensive. - if (ch.parent() != null - && childPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { - try { - childPipeline.addFirst( - Http2PingCloseRewrapHandler.HANDLER_NAME, - Http2PingCloseRewrapHandler.INSTANCE); - } catch (IllegalArgumentException ignored) { - // Benign duplicate install: another install path may have - // added the handler between the get()==null check above and - // addFirst(). A duplicate handler name is the only cause. - } - } - } - }); + // Install a @Sharable head-of-pipeline rewrap handler on each H2 + // child-stream pipeline. STREAM_CONFIGURED is the per-child-stream + // lifecycle event reactor-netty fires once when a stream is opened, so + // connection.channel() here is the child stream (its parent is the + // parent TCP channel). doOnConnected cannot be used for this install: + // it fires only on the parent TCP channel (State.CONFIGURED) and never + // on a child stream, so a ch.parent() != null gate inside doOnConnected + // would never be satisfied and the handler would never install. + // + // The rewrap MUST live on the child stream pipeline, not the parent. + // When Http2PingHandler closes the parent (TCP) channel after + // consecutive PING-ACK timeouts or PING-send failures, the H2 multiplex + // codec propagates channelInactive to each child stream independently. + // Child streams are separate Netty Channels, so that close does not + // surface on the parent pipeline -- reactor-netty's per-stream + // HttpClientOperations turns the child channelInactive into a bare + // PrematureCloseException. This head-of-child-pipeline handler + // intercepts channelInactive first and fires exceptionCaught with a + // typed Http2PingTimeoutChannelClosedException before HttpClientOperations + // observes the close, so the response Mono fails with the typed + // exception. The rest of the stack maps that to + // GATEWAY_HTTP2_PING_TIMEOUT_CHANNEL_CLOSED so ClientRetryPolicy can + // suppress region mark-down. + // + // The install/skip decision (state gate, PING-health gate, parent and + // idempotency guards) is extracted into + // installHttp2PingCloseRewrapHandlerIfNeeded so it can be unit-tested + // without a live HTTP/2 connection. + .observe((connection, state) -> + installHttp2PingCloseRewrapHandlerIfNeeded(connection.channel(), state, http2Cfg)); + } + } + + /** + * Installs the {@link Http2PingCloseRewrapHandler} at the head of an HTTP/2 + * child-stream pipeline when, and only when, PING-health is effectively enabled. + * Invoked from the {@code .observe(...)} hook in + * {@link #configureChannelPipelineHandlers()} for every connection-lifecycle event; + * see that hook for why the install must happen on the child stream at + * {@link HttpClientState#STREAM_CONFIGURED} rather than via {@code doOnConnected} on + * the parent TCP channel. + * + *

This is a no-op unless all of the following hold: + *

    + *
  • {@code state} is {@link HttpClientState#STREAM_CONFIGURED} (a child stream was + * just opened);
    • + *
  • {@link Http2PingHandler#isPingHealthEffectivelyEnabled(Http2ConnectionConfig)} + * is {@code true} -- the rewrap handler only has a PING-timeout close signal to + * translate while the PING sender is active, so when PING-health is disabled (via + * {@code COSMOS.HTTP2_PING_HEALTH_ENABLED=false}, a non-positive PING interval, or + * HTTP/2 itself being disabled) the install is skipped and the kill-switch is a + * true revert-to-baseline with no extra per-stream pipeline hop;
    • + *
  • {@code channel.parent()} is non-null (defensive: STREAM_CONFIGURED already + * implies a child stream) and the handler is not already installed.
    • + *
+ * The PING-health predicate is evaluated only after the state check so the common + * non-stream lifecycle events stay off this path. The gate is evaluated once per stream + * at install time; toggling the kill-switch on at runtime only affects streams + * configured afterwards, and since streams are single-use behavior converges within one + * stream lifetime. + * + * @param channel the channel reactor-netty associated with the lifecycle event (the + * child stream when {@code state} is STREAM_CONFIGURED) + * @param state the connection-lifecycle state being observed + * @param http2Cfg the per-client HTTP/2 configuration backing the PING-health gate + */ + static void installHttp2PingCloseRewrapHandlerIfNeeded( + Channel channel, + ConnectionObserver.State state, + Http2ConnectionConfig http2Cfg) { + + if (state != HttpClientState.STREAM_CONFIGURED + || !Http2PingHandler.isPingHealthEffectivelyEnabled(http2Cfg)) { + return; + } + + ChannelPipeline childPipeline = channel.pipeline(); + // STREAM_CONFIGURED implies a child stream, so channel.parent() is the parent TCP + // channel; the null-check is defensive. + if (channel.parent() != null + && childPipeline.get(Http2PingCloseRewrapHandler.HANDLER_NAME) == null) { + try { + childPipeline.addFirst( + Http2PingCloseRewrapHandler.HANDLER_NAME, + Http2PingCloseRewrapHandler.INSTANCE); + } catch (IllegalArgumentException ignored) { + // Benign duplicate install: another install path may have added the handler + // between the get()==null check above and addFirst(). A duplicate handler + // name is the only cause. + } } } From a62c5edcc403b18cf740bb980b9987fe7411d55f Mon Sep 17 00:00:00 2001 From: Abhijeet Mohanty Date: Sun, 7 Jun 2026 18:52:44 -0400 Subject: [PATCH 6/6] Update CHANGELOG.md --- sdk/cosmos/azure-cosmos/CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/cosmos/azure-cosmos/CHANGELOG.md b/sdk/cosmos/azure-cosmos/CHANGELOG.md index 3382ffc901d5..4dfa21796c50 100644 --- a/sdk/cosmos/azure-cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure-cosmos/CHANGELOG.md @@ -13,7 +13,6 @@ * Fixed region name normalization for preferred and excluded regions — non-canonical inputs (e.g., `"westus3"`, `"WEST US 3"`) are now mapped to the canonical form. Also fixed a case-sensitive exclude-region check in PPCB reevaluate logic. - See [PR 49090](https://github.com/Azure/azure-sdk-for-java/pull/49090) * Fixed `UnsupportedOperationException` when using `readManyByPartitionKeys` for empty pages. - See [PR 49311](https://github.com/Azure/azure-sdk-for-java/pull/49311) * Fixed silent drift in `CosmosChangeFeedRequestOptions` when resuming from a continuation token via `byPage(savedContinuation)`. Previously only `maxPrefetchPageCount` and `throughputControlGroupName` were inherited onto the rebuilt impl; `endLSN`, `customSerializer`, `excludeRegions`, `readConsistencyStrategy`, `completeAfterAllCurrentChangesRetrieved`, and other caller-supplied configuration were silently dropped. All non-token-encoded fields are now propagated. - See [PR 49276](https://github.com/Azure/azure-sdk-for-java/pull/49276) -* Fixed the HTTP/2 PING-timeout rewrap handler (`Http2PingCloseRewrapHandler`, introduced in [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095)) never being installed. It is now added at the head of each HTTP/2 child-stream pipeline via reactor-netty's `.observe(...)` hook on the `STREAM_CONFIGURED` lifecycle event. The prior `.doOnConnected()` install fired only on the parent TCP channel and never reached child streams, so PING-timeout parent closes surfaced as a bare `PrematureCloseException` instead of the typed `Http2PingTimeoutChannelClosedException` (preventing `ClientRetryPolicy` from suppressing region mark-down). The install is gated on `Http2PingHandler.isPingHealthEffectivelyEnabled` so disabling PING-health (`COSMOS.HTTP2_PING_HEALTH_ENABLED=false`) is a true revert-to-baseline. - See [PR 49403](https://github.com/Azure/azure-sdk-for-java/pull/49403) #### Other Changes * Added HTTP/2 PING keepalive (default ON) for Gateway service endpoints to detect silently-broken connections. - See [PR 49095](https://github.com/Azure/azure-sdk-for-java/pull/49095)