Add exception handler on HTTP/2 parent channel to suppress WARN logs#48890
Add exception handler on HTTP/2 parent channel to suppress WARN logs#48890jeet1995 wants to merge 7 commits intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Netty channel handler to suppress noisy “exceptionCaught reached tail of pipeline” WARN logs on HTTP/2 parent (TCP) connections in Cosmos’ Reactor Netty transport, while preserving WARN-level signal when exceptions may impact in-flight HTTP/2 streams.
Changes:
- Install an HTTP/2 parent-channel
exceptionCaughthandler fromReactorNettyClientwhen HTTP/2 is enabled. - Add
Http2ParentChannelExceptionHandlerthat consumes parent-channel exceptions and logs at DEBUG vs WARN based on active stream count and channel activity. - Add EmbeddedChannel-based unit tests covering exception consumption behavior, and update changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java | Adds logic to install the new handler onto the HTTP/2 parent channel pipeline. |
| sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2ParentChannelExceptionHandler.java | New handler that consumes parent-channel exceptions and logs based on connection state. |
| sdk/cosmos/azure-cosmos/CHANGELOG.md | Documents the fix in the unreleased section. |
| sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/implementation/http/Http2ParentChannelExceptionHandlerTest.java | New unit tests validating the handler’s exception consumption behavior. |
| sdk/cosmos/azure-cosmos-tests/pom.xml | Enables surefire tests and includes trailing whitespace changes. |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
In HTTP/2, reactor-netty multiplexes streams on a shared parent TCP connection. The parent channel pipeline has no ChannelOperationsHandler (unlike HTTP/1.1), so TCP-level exceptions like Connection reset by peer (ECONNRESET) propagate to Netty's TailContext, which logs them as WARN. This adds Http2ParentChannelExceptionHandler to the parent channel via doOnConnected (accessing channel.parent()). The handler consumes exceptions at DEBUG level WITHOUT closing the channel or altering connection lifecycle, matching HTTP/1.1 logging behavior. Changes: - Handler logs cause.toString() (not getMessage()) for null-safe diagnostics - Defensive try-catch for duplicate handler name on concurrent stream creation - Before/after verified with EmbeddedChannel unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…toString(), update changelog Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d68fa5c to
2a3b5b2
Compare
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sdkReviewAgent |
| } | ||
| } else { | ||
| // Active streams on a live channel — exception may affect in-flight requests. | ||
| logger.warn( |
There was a problem hiding this comment.
Do we need to add tests to check the activeStrems > 0 path?
There was a problem hiding this comment.
Yes the WARN path (activeStreams > 0 on a live channel) is the most operationally important branch and currently has zero test coverage. All 5 tests exercise only the activeStreams == 0 (DEBUG) path.
A pragmatic way to cover the WARN path without needing real H2 stream negotiation: create an EmbeddedChannel with only the handler (no Http2FrameCodec). When the codec is absent, getActiveStreamCount() returns -1, and since -1 != 0 and EmbeddedChannel.isActive() == true, the condition at line 49 evaluates to false, taking the WARN branch.
`java
@test(groups = "unit")
public void withHandler_codecAbsent_fallsBackToWarnPath() {
// Without Http2FrameCodec, getActiveStreamCount() returns -1.
// -1 != 0 AND channelActive == true takes WARN path (safe default).
EmbeddedChannel channel = new EmbeddedChannel(
new Http2ParentChannelExceptionHandler());
channel.pipeline().fireExceptionCaught(
new IOException("Connection reset by peer"));
// Exception is still consumed (does not reach TailContext)
channel.checkException();
assertThat(channel.isOpen()).isTrue();
channel.finishAndReleaseAll();
}
`
This single test covers two gaps simultaneously:
- The
-1fallback behavior ofgetActiveStreamCount()when the codec is unavailable - The WARN branch execution (the
elseat line 57)
It doesn't verify the actual log level (that would require a log capture mechanism), but it proves the handler still consumes exceptions on the WARN path rather than letting them reach TailContext.
There was a problem hiding this comment.
Good catch @mbhaskar. Added tests for > 0 and -1 (when Http2FrameCodec isn't installed) activeStreams path.
Address Bhaskar's review: add two tests covering the else branch where activeStreams > 0 on an active channel, exercising the WARN log path. - withHandler_activeStreams_consumedAtWarn: creates an active H2 stream via codec.connection().local().createStream(), fires an exception, and verifies it is consumed (does not reach TailContext). - withHandler_activeStreams_channelNotClosed: same setup, verifies the handler does not close the channel even with active streams. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Review complete (32:05) No new comments — existing review coverage is sufficient. Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
When Http2FrameCodec is absent from the pipeline, getActiveStreamCount() returns -1. Since -1 != 0 and channelActive == true, the handler takes the safe WARN path. This test covers that fallback behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run java - cosmos - tests |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
|
||
| @Override | ||
| public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { | ||
| int activeStreams = getActiveStreamCount(ctx); |
There was a problem hiding this comment.
I am wondering when exception happens, should we proactively calling ctx.close() to force close any broken connections. From reading, seems not all exceptions will cause connection to close. so I am any edge cases we need to take care here.
and another question is should we swallow all exceptions, what if for Error cases, should we still swallow? should we throw again?
Problem
Customers see noisy Netty WARN logs in HTTP/2 scenarios:
Root Cause
In HTTP/2, reactor-netty multiplexes streams on a shared parent TCP connection. The parent and child channels have different pipeline structures:
HTTP/1.1 pipeline (single channel — no leak to TailContext):
HTTP/2 parent channel pipeline (BEFORE fix — leak to TailContext):
HTTP/2 parent channel pipeline (AFTER fix):
HTTP/2 child stream channel pipeline (unchanged):
Design: Connection-State-Based Log Level
The handler consumes all exceptions on the parent channel (no exception type filtering). The log level is determined by connection state:
activeStreams == 0OR!channelActive.Active stream count is retrieved via
Http2FrameCodec.connection().numActiveStreams()on the same parent channel pipeline. Falls back to -1 if the codec is unavailable, which takes the safe WARN path.Why no exception type filtering?
By the time any exception reaches our handler, all upstream handlers (
Http2FrameCodec,Http2MultiplexHandler) have already handled the protocol actions (GOAWAY, stream reset, child channel error delivery). The exception reaching TailContext is an echo of already-handled work, regardless of type. Connection state (active streams + channel activity) is the only dimension that determines whether the exception has diagnostic value.Why OR (not AND) for the DEBUG condition?
Either condition alone is sufficient:
activeStreams == 0— no in-flight requests affected, regardless of channel state!channelActive— channel is already dead, any active streams will fail through their Reactor subscribers independentlyTesting
5 EmbeddedChannel unit tests with production-matching pipeline (
Http2FrameCodec→Http2MultiplexHandler→ handler):withoutHandler_exceptionReachesTailwithHandler_zeroActiveStreams_consumedAtDebugwithHandler_exceptionDoesNotCloseChannelwithHandler_runtimeException_zeroActiveStreams_consumedwithHandler_npe_zeroActiveStreams_consumedNote: The
!channelActivebranch cannot be unit-tested with EmbeddedChannel becausedisconnect()tears down the pipeline beforefireExceptionCaughtcan reach handlers. In production,exceptionCaught()fires while the channel is transitioning to inactive.Impact
exceptionCaught()— Netty@Skipoptimization bypasses it for all hot-path events