-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add exception handler on HTTP/2 parent channel to suppress WARN logs #48687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
152 changes: 152 additions & 0 deletions
152
...est/java/com/azure/cosmos/implementation/http/Http2ParentChannelExceptionHandlerTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.cosmos.implementation.http; | ||
|
|
||
| import io.netty.channel.ChannelInboundHandlerAdapter; | ||
| import io.netty.channel.embedded.EmbeddedChannel; | ||
| import io.netty.handler.codec.http2.Http2FrameCodec; | ||
| import io.netty.handler.codec.http2.Http2FrameCodecBuilder; | ||
| import io.netty.handler.codec.http2.Http2MultiplexHandler; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| /** | ||
| * Verifies that {@link Http2ParentChannelExceptionHandler} uses connection | ||
| * state — active stream count and channel activity — to determine whether | ||
| * exceptions are logged at DEBUG (suppressed) or WARN (preserved). | ||
| * Exception type is NOT a filtering dimension. | ||
| * | ||
| * The EmbeddedChannel is configured to mirror the production HTTP/2 parent | ||
| * channel pipeline: | ||
| * <pre> | ||
| * Http2FrameCodec → Http2MultiplexHandler → Http2ParentChannelExceptionHandler → TailContext | ||
| * </pre> | ||
| * (SslHandler is omitted because it requires an SSLContext and is not relevant | ||
| * to exception propagation behavior.) | ||
| * | ||
| * {@code checkException()} re-throws any exception that reached the pipeline tail. | ||
| */ | ||
| public class Http2ParentChannelExceptionHandlerTest { | ||
|
|
||
| /** | ||
| * Creates an EmbeddedChannel matching the production HTTP/2 parent channel | ||
| * pipeline (minus SslHandler): Http2FrameCodec → Http2MultiplexHandler → | ||
| * Http2ParentChannelExceptionHandler. | ||
| */ | ||
| private static EmbeddedChannel createH2ParentChannel(boolean withExceptionHandler) { | ||
| Http2FrameCodec codec = Http2FrameCodecBuilder.forClient() | ||
| .autoAckSettingsFrame(true) | ||
| .build(); | ||
|
|
||
| Http2MultiplexHandler multiplexHandler = new Http2MultiplexHandler( | ||
| new ChannelInboundHandlerAdapter()); | ||
|
|
||
| if (withExceptionHandler) { | ||
| return new EmbeddedChannel(codec, multiplexHandler, | ||
| new Http2ParentChannelExceptionHandler()); | ||
| } else { | ||
| return new EmbeddedChannel(codec, multiplexHandler); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * BEFORE fix — without the handler, exceptions reach the pipeline tail. | ||
| * EmbeddedChannel's checkException() re-throws the unhandled exception, | ||
| * proving it reached Netty's TailContext (which in production logs as WARN). | ||
| */ | ||
| @Test(groups = "unit") | ||
| public void withoutHandler_exceptionReachesTail() { | ||
| EmbeddedChannel channel = createH2ParentChannel(false); | ||
|
|
||
| channel.pipeline().fireExceptionCaught( | ||
| new IOException("Connection reset by peer")); | ||
|
|
||
| assertThatThrownBy(channel::checkException) | ||
| .isInstanceOf(IOException.class) | ||
| .hasMessageContaining("Connection reset by peer"); | ||
|
|
||
| channel.finishAndReleaseAll(); | ||
| } | ||
|
|
||
| /** | ||
| * With handler — exception on idle connection (0 active streams) is | ||
| * consumed at DEBUG. The suppression is based on connection state | ||
| * (no active streams), not exception type. | ||
| * | ||
| * In production, channel.isActive() transitions to false during the | ||
| * RST handling cycle, satisfying the OR condition. In EmbeddedChannel | ||
| * we can only verify the activeStreams == 0 branch. | ||
| */ | ||
| @Test(groups = "unit") | ||
| public void withHandler_zeroActiveStreams_consumedAtDebug() { | ||
| EmbeddedChannel channel = createH2ParentChannel(true); | ||
|
|
||
| Http2FrameCodec codec = channel.pipeline().get(Http2FrameCodec.class); | ||
| assertThat(codec).isNotNull(); | ||
| assertThat(codec.connection().numActiveStreams()).isEqualTo(0); | ||
|
|
||
| channel.pipeline().fireExceptionCaught( | ||
| new IOException("recvAddress(..) failed with error(-104): Connection reset by peer")); | ||
|
|
||
| // Exception consumed — does NOT reach tail | ||
| channel.checkException(); | ||
|
|
||
| channel.finishAndReleaseAll(); | ||
| } | ||
|
|
||
| /** | ||
| * Handler does not close the channel — connection lifecycle is managed | ||
| * by reactor-netty's pool eviction, not by this handler. | ||
| */ | ||
| @Test(groups = "unit") | ||
| public void withHandler_exceptionDoesNotCloseChannel() { | ||
| EmbeddedChannel channel = createH2ParentChannel(true); | ||
|
|
||
| assertThat(channel.isActive()).isTrue(); | ||
|
|
||
| channel.pipeline().fireExceptionCaught( | ||
| new IOException("Connection reset by peer")); | ||
|
|
||
| channel.checkException(); | ||
| assertThat(channel.isOpen()).isTrue(); | ||
|
|
||
| channel.finishAndReleaseAll(); | ||
| } | ||
|
|
||
| /** | ||
| * RuntimeException on idle connection is also consumed — suppression | ||
| * is based on connection state, not exception type. | ||
| */ | ||
| @Test(groups = "unit") | ||
| public void withHandler_runtimeException_zeroActiveStreams_consumed() { | ||
| EmbeddedChannel channel = createH2ParentChannel(true); | ||
|
|
||
| channel.pipeline().fireExceptionCaught( | ||
| new RuntimeException("Unexpected state error")); | ||
|
|
||
| channel.checkException(); | ||
|
|
||
| channel.finishAndReleaseAll(); | ||
| } | ||
|
|
||
| /** | ||
| * NullPointerException on idle connection is also consumed — same | ||
| * connection-state-based suppression regardless of exception type. | ||
| */ | ||
| @Test(groups = "unit") | ||
| public void withHandler_npe_zeroActiveStreams_consumed() { | ||
| EmbeddedChannel channel = createH2ParentChannel(true); | ||
|
|
||
| channel.pipeline().fireExceptionCaught( | ||
| new NullPointerException("handler bug")); | ||
|
|
||
| channel.checkException(); | ||
|
|
||
| channel.finishAndReleaseAll(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
...rc/main/java/com/azure/cosmos/implementation/http/Http2ParentChannelExceptionHandler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package com.azure.cosmos.implementation.http; | ||
|
|
||
| import io.netty.channel.ChannelHandlerContext; | ||
| import io.netty.channel.ChannelInboundHandlerAdapter; | ||
| import io.netty.handler.codec.http2.Http2FrameCodec; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Exception handler for the HTTP/2 parent (TCP) channel pipeline. | ||
| * <p> | ||
| * In HTTP/2, reactor-netty multiplexes streams on a shared parent TCP connection. | ||
| * Child stream channels have {@code ChannelOperationsHandler} which catches exceptions | ||
| * and fails the active subscriber (matching HTTP/1.1 behavior). However, the parent | ||
| * channel has no such handler — exceptions propagate to Netty's {@code TailContext} | ||
| * which logs them as WARN ("An exceptionCaught() event was fired, and it reached at | ||
| * the tail of the pipeline"). | ||
| * <p> | ||
| * This handler consumes all exceptions on the parent channel and uses connection | ||
| * state to decide the log level: | ||
| * <ul> | ||
| * <li><b>DEBUG</b> — when {@code activeStreams == 0} OR {@code !channelActive}. | ||
| * No in-flight requests are affected (e.g., TCP RST from LB idle timeout, | ||
| * post-close cleanup).</li> | ||
| * <li><b>WARN</b> — when active streams exist on a live channel. The exception | ||
| * may affect in-flight requests and is worth alerting on.</li> | ||
| * </ul> | ||
| * <p> | ||
| * The handler does NOT close the channel or alter connection lifecycle — reactor-netty | ||
| * and the connection pool's eviction predicate ({@code !channel.isActive()}) handle that | ||
| * independently. | ||
| * | ||
| * @see ReactorNettyClient#configureChannelPipelineHandlers() | ||
| */ | ||
| final class Http2ParentChannelExceptionHandler extends ChannelInboundHandlerAdapter { | ||
|
|
||
| static final String HANDLER_NAME = "cosmosH2ParentExceptionHandler"; | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(Http2ParentChannelExceptionHandler.class); | ||
|
|
||
| @Override | ||
| public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { | ||
| int activeStreams = getActiveStreamCount(ctx); | ||
| boolean channelActive = ctx.channel().isActive(); | ||
|
|
||
| if (activeStreams == 0 || !channelActive) { | ||
| // No active streams OR channel already inactive — exception is noise | ||
| // (e.g., TCP RST from LB idle timeout, post-close cleanup). | ||
| if (logger.isDebugEnabled()) { | ||
| logger.debug( | ||
| "Exception on HTTP/2 parent connection [id:{}, activeStreams={}, channelActive={}]: {}", | ||
| ctx.channel().id().asShortText(), activeStreams, channelActive, cause.toString(), cause); | ||
| } | ||
| } else { | ||
| // Active streams on a live channel — exception may affect in-flight requests. | ||
| logger.warn( | ||
| "Exception on HTTP/2 parent connection [id:{}, activeStreams={}, channelActive={}]: {}", | ||
| ctx.channel().id().asShortText(), activeStreams, channelActive, cause.toString(), cause); | ||
| } | ||
| } | ||
|
|
||
| private static int getActiveStreamCount(ChannelHandlerContext ctx) { | ||
| try { | ||
| Http2FrameCodec codec = ctx.pipeline().get(Http2FrameCodec.class); | ||
| if (codec != null) { | ||
| return codec.connection().numActiveStreams(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // Codec not available or connection already torn down | ||
| } | ||
| return -1; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces new behavior (consuming parent-channel exceptions and closing the parent connection) without accompanying test coverage. There are existing Netty/transport tests in azure-cosmos-tests (e.g., ones that use EmbeddedChannel); please add a unit/integration test that asserts the handler is installed on the H2 parent pipeline and that an exception on the parent is consumed (no TailContext WARN) and results in the parent channel closing.