From 2c64f235332d17ab510040cebc3db951310f9ad7 Mon Sep 17 00:00:00 2001 From: Pavel Ptashyts <49400901+pavel-ptashyts@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:31:56 +0200 Subject: [PATCH 1/2] Mark an HTTP/2 connection instead of looking isHttp2 asked the pipeline for the multiplex handler by name, and the write path asks isHttp2 of every request: once to decide whether to store the per-request future on the channel, once to route the write. A pipeline lookup walks the handlers comparing names, and an HTTP/1.1 connection, having no such handler, is walked to the end to say no, which is the common case for anyone not using HTTP/2. In one profile of a client with HTTP/2 disabled, DefaultChannelPipeline.context0 took 83 CPU samples on that account alone. The multiplex handler is installed in exactly one place, so a channel attribute is set beside it and isHttp2 reads that: a binary search over integer keys in a small array rather than a walk with a string compare per handler. hasAttr rather than attr().get(), which would add an entry to the attribute map of every HTTP/1.1 channel just to find none. Not a config check. Reading isHttp2Enabled first would be cheaper still, but the two can disagree: NettyConnectListener upgrades on the ALPN result alone, and a caller who supplies an SslContext or an SslEngineFactory of their own controls what ALPN advertises whatever the config says - which the WebSocket guard beside it already accounts for. A request would then be written as HTTP/1.1 onto an HTTP/2 pipeline. The attribute costs nothing extra and cannot disagree, since it is set where the handler is. The attribute cannot go stale either: nothing removes the multiplex handler from a pipeline, so there is no downgrade for the two to differ across. ChannelManagerHttp2MarkerTest pins them together, so a later change to the upgrade cannot set one without the other. Removing the attribute fails that test and 46 of the 52 in BasicHttp2Test, the write path having routed HTTP/2 connections down the HTTP/1.1 branch. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 --- .../netty/channel/ChannelManager.java | 14 +++- .../ChannelManagerHttp2MarkerTest.java | 84 +++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java index f305fb3f3..95ce64f57 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -58,6 +58,7 @@ import io.netty.resolver.AddressResolver; import io.netty.resolver.AddressResolverGroup; import io.netty.resolver.NameResolver; +import io.netty.util.AttributeKey; import io.netty.util.Timer; import io.netty.util.concurrent.DefaultThreadFactory; import io.netty.util.concurrent.Future; @@ -125,6 +126,8 @@ public class ChannelManager { public static final String LOGGING_HANDLER = "logging"; public static final String HTTP2_FRAME_CODEC = "http2-frame-codec"; public static final String HTTP2_MULTIPLEX = "http2-multiplex"; + // Set beside HTTP2_MULTIPLEX and nowhere else, so that isHttp2 can answer without a pipeline lookup. + private static final AttributeKey HTTP2_CONNECTION_ATTRIBUTE = AttributeKey.valueOf("http2Connection"); public static final String AHC_HTTP2_HANDLER = "ahc-http2"; private static final String TARGET_SSL_HANDLER = "target-ssl"; private static final Logger LOGGER = LoggerFactory.getLogger(ChannelManager.class); @@ -1027,10 +1030,16 @@ protected void initChannel(Channel channel) throws Exception { } /** - * Checks whether the given channel is an HTTP/2 connection (i.e. has the HTTP/2 multiplex handler installed). + * Checks whether the given channel is an HTTP/2 connection: the parent that multiplexes streams, not one of + * its stream children, whose own pipelines carry neither the multiplex handler nor this attribute. + *

+ * Answered from an attribute rather than by looking {@link #HTTP2_MULTIPLEX} up in the pipeline. The two are + * set together and so always agree, but a pipeline lookup compares handler names down the chain, and an + * HTTP/1.1 connection, which has no such handler, is walked to the end to say no. The write path asks this + * of every request. */ public static boolean isHttp2(Channel channel) { - return channel.pipeline().get(HTTP2_MULTIPLEX) != null; + return channel.hasAttr(HTTP2_CONNECTION_ATTRIBUTE); } /** @@ -1096,6 +1105,7 @@ protected void initChannel(Channel ch) { pipeline.addLast(HTTP2_FRAME_CODEC, frameCodec); pipeline.addLast(HTTP2_MULTIPLEX, multiplexHandler); + pipeline.channel().attr(HTTP2_CONNECTION_ATTRIBUTE).set(Boolean.TRUE); // Attach HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining Http2ConnectionState state = new Http2ConnectionState(); diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java new file mode 100644 index 000000000..e2fc832cd --- /dev/null +++ b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java @@ -0,0 +1,84 @@ +/* + * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.asynchttpclient.netty.channel; + +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.util.HashedWheelTimer; +import io.netty.util.Timer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.asynchttpclient.Dsl.config; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * {@link ChannelManager#isHttp2(io.netty.channel.Channel)} answers from an attribute, while the thing it stands + * for is the multiplex handler in the pipeline. These pin the two together: either both say HTTP/2 or neither + * does, whichever way a later change to the upgrade sets them. + */ +class ChannelManagerHttp2MarkerTest { + + private ChannelManager channelManager; + private Timer timer; + private EmbeddedChannel channel; + + @BeforeEach + void setUp() { + timer = new HashedWheelTimer(); + channelManager = new ChannelManager(config().build(), timer); + channel = new EmbeddedChannel(); + } + + @AfterEach + void tearDown() { + channel.finishAndReleaseAll(); + timer.stop(); + } + + @Test + void aConnectionThatWasNeverUpgradedIsNotHttp2() { + assertNull(channel.pipeline().get(ChannelManager.HTTP2_MULTIPLEX), + "an untouched pipeline should not carry the multiplex handler"); + assertFalse(ChannelManager.isHttp2(channel), "and should not be reported as HTTP/2"); + } + + @Test + void upgradingAConnectionBothInstallsTheHandlerAndReportsHttp2() { + channelManager.upgradePipelineToHttp2(channel.pipeline()); + + assertNotNull(channel.pipeline().get(ChannelManager.HTTP2_MULTIPLEX), + "the upgrade should install the multiplex handler"); + assertTrue(ChannelManager.isHttp2(channel), "and should report the connection as HTTP/2"); + } + + @Test + void aStreamChannelIsNotItsParentsConnection() { + // Nothing marks a stream child, and its own pipeline carries no multiplex handler either, so the two + // agree here as well: a stream is not the connection that multiplexes it. + channelManager.upgradePipelineToHttp2(channel.pipeline()); + EmbeddedChannel stream = new EmbeddedChannel(); + try { + assertNull(stream.pipeline().get(ChannelManager.HTTP2_MULTIPLEX)); + assertFalse(ChannelManager.isHttp2(stream)); + } finally { + stream.finishAndReleaseAll(); + } + } +} From 5bcb5f2d92954cf70da9be6832bb89f506efda3c Mon Sep 17 00:00:00 2001 From: Pavel Ptashyts <49400901+pavel-ptashyts@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:53:45 +0200 Subject: [PATCH 2/2] Route HTTP/2 on the state already attached Review feedback. The previous commit added an attribute of its own to stand for an HTTP/2 connection, but the connection already carries one: upgradePipelineToHttp2 attaches an Http2ConnectionState ten lines below the multiplex handler, and writeHttp2Request reads it back as its first statement. Routing on that drops the second marker, and with it the question of keeping two in sync. writeRequest reads the state once and hands it to writeHttp2Request, which no longer looks it up again, and the null check that guarded that lookup goes too: its only caller now routes on the state being there. Neither the state nor the multiplex handler is ever taken away by this client, so the two agree for as long as a connection lives. They part after Netty's own teardown removes the handlers on close, where isHttp2 keeps saying HTTP/2 and a pipeline lookup would have stopped: both callers are behind an active-channel check, so nothing asks by then. The test builds one ChannelManager for the class rather than one per test - an SslContext and an event loop group each time, for state that lives on the channel - and closes it, which it was not doing: the fork gained file descriptors it never gave back. The stream case opened a bare EmbeddedChannel, which has no parent and so asserted what the first case already did; it opens a real stream child now, which pins the part that is actually new, a stream not inheriting the connection state of its parent. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 --- .../netty/channel/ChannelManager.java | 20 +++--- .../netty/request/NettyRequestSender.java | 19 +++--- .../ChannelManagerHttp2MarkerTest.java | 61 ++++++++++++++----- 3 files changed, 66 insertions(+), 34 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java index 95ce64f57..954f7ec9f 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -58,7 +58,6 @@ import io.netty.resolver.AddressResolver; import io.netty.resolver.AddressResolverGroup; import io.netty.resolver.NameResolver; -import io.netty.util.AttributeKey; import io.netty.util.Timer; import io.netty.util.concurrent.DefaultThreadFactory; import io.netty.util.concurrent.Future; @@ -126,8 +125,6 @@ public class ChannelManager { public static final String LOGGING_HANDLER = "logging"; public static final String HTTP2_FRAME_CODEC = "http2-frame-codec"; public static final String HTTP2_MULTIPLEX = "http2-multiplex"; - // Set beside HTTP2_MULTIPLEX and nowhere else, so that isHttp2 can answer without a pipeline lookup. - private static final AttributeKey HTTP2_CONNECTION_ATTRIBUTE = AttributeKey.valueOf("http2Connection"); public static final String AHC_HTTP2_HANDLER = "ahc-http2"; private static final String TARGET_SSL_HANDLER = "target-ssl"; private static final Logger LOGGER = LoggerFactory.getLogger(ChannelManager.class); @@ -1031,15 +1028,16 @@ protected void initChannel(Channel channel) throws Exception { /** * Checks whether the given channel is an HTTP/2 connection: the parent that multiplexes streams, not one of - * its stream children, whose own pipelines carry neither the multiplex handler nor this attribute. + * its stream children, which carry neither the multiplex handler nor connection state of their own. *

- * Answered from an attribute rather than by looking {@link #HTTP2_MULTIPLEX} up in the pipeline. The two are - * set together and so always agree, but a pipeline lookup compares handler names down the chain, and an - * HTTP/1.1 connection, which has no such handler, is walked to the end to say no. The write path asks this - * of every request. + * Answered from the {@link Http2ConnectionState} attached to the connection rather than by looking + * {@link #HTTP2_MULTIPLEX} up in the pipeline. The two are attached together, in + * {@link #upgradePipelineToHttp2}, and neither is ever taken away, so they say the same thing; but a + * pipeline lookup compares handler names down the chain, and an HTTP/1.1 connection, which has no such + * handler, is walked to the end to say no. The write path asks this of every request. */ public static boolean isHttp2(Channel channel) { - return channel.hasAttr(HTTP2_CONNECTION_ATTRIBUTE); + return channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get() != null; } /** @@ -1105,9 +1103,9 @@ protected void initChannel(Channel ch) { pipeline.addLast(HTTP2_FRAME_CODEC, frameCodec); pipeline.addLast(HTTP2_MULTIPLEX, multiplexHandler); - pipeline.channel().attr(HTTP2_CONNECTION_ATTRIBUTE).set(Boolean.TRUE); - // Attach HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining + // Attach HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining. Its + // presence is also what marks the connection as HTTP/2; see isHttp2. Http2ConnectionState state = new Http2ConnectionState(); int configMaxStreams = config.getHttp2MaxConcurrentStreams(); if (configMaxStreams > 0) { diff --git a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java index c142bc62a..a9beb025a 100755 --- a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java +++ b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java @@ -703,9 +703,12 @@ public void writeRequest(NettyResponseFuture future, Channel channel) { return; } - // Route to HTTP/2 path if the parent channel has the HTTP/2 multiplex handler installed - if (ChannelManager.isHttp2(channel)) { - writeHttp2Request(future, channel); + // Route to HTTP/2 when the connection carries HTTP/2 state, which is attached where the multiplex + // handler is. Read here rather than asked of ChannelManager.isHttp2, because the HTTP/2 path needs the + // state itself and would otherwise look up what this line has already found. + Http2ConnectionState http2State = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); + if (http2State != null) { + writeHttp2Request(future, channel, http2State); return; } @@ -772,10 +775,12 @@ public void writeRequest(NettyResponseFuture future, Channel channel) { * The stream child channel has the {@link org.asynchttpclient.netty.handler.Http2Handler} installed * and the {@link NettyResponseFuture} attached to it, mirroring the HTTP/1.1 channel model. */ - private void writeHttp2Request(NettyResponseFuture future, Channel parentChannel) { - Http2ConnectionState state = parentChannel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); - - if (state != null && !state.tryAcquireStream()) { + /** + * @param state the connection's HTTP/2 state, which is what identified it as an HTTP/2 connection in the + * first place, so the caller has it in hand + */ + private void writeHttp2Request(NettyResponseFuture future, Channel parentChannel, Http2ConnectionState state) { + if (!state.tryAcquireStream()) { if (state.isDraining()) { // Connection is draining from GOAWAY — fail the future so it retries on a new connection. // Don't close the parent channel since it may still have active streams. sendHttp2Frames diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java index e2fc832cd..16e8fcb28 100644 --- a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2MarkerTest.java @@ -15,10 +15,15 @@ */ package org.asynchttpclient.netty.channel; +import io.netty.channel.Channel; +import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.codec.http2.Http2StreamChannelBootstrap; import io.netty.util.HashedWheelTimer; import io.netty.util.Timer; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -29,27 +34,45 @@ import static org.junit.jupiter.api.Assertions.assertTrue; /** - * {@link ChannelManager#isHttp2(io.netty.channel.Channel)} answers from an attribute, while the thing it stands - * for is the multiplex handler in the pipeline. These pin the two together: either both say HTTP/2 or neither - * does, whichever way a later change to the upgrade sets them. + * {@link ChannelManager#isHttp2(Channel)} answers from the {@link Http2ConnectionState} attached to a + * connection, while the thing it stands for is the multiplex handler in the pipeline. These pin the two + * together: either both say HTTP/2 or neither does, whichever way a later change to the upgrade attaches them. */ class ChannelManagerHttp2MarkerTest { - private ChannelManager channelManager; - private Timer timer; + // One per class: the upgrade is what is under test and it needs a ChannelManager only to be called. Building + // one per test costs an SslContext and an event loop group each time, for state that lives on the channel. + private static ChannelManager channelManager; + private static Timer timer; + private EmbeddedChannel channel; - @BeforeEach - void setUp() { + @BeforeAll + static void startManager() { timer = new HashedWheelTimer(); channelManager = new ChannelManager(config().build(), timer); + } + + @AfterAll + static void stopManager() { + if (channelManager != null) { + channelManager.close(); + } + if (timer != null) { + timer.stop(); + } + } + + @BeforeEach + void setUp() { channel = new EmbeddedChannel(); } @AfterEach void tearDown() { - channel.finishAndReleaseAll(); - timer.stop(); + if (channel != null) { + channel.finishAndReleaseAll(); + } } @Test @@ -69,16 +92,22 @@ void upgradingAConnectionBothInstallsTheHandlerAndReportsHttp2() { } @Test - void aStreamChannelIsNotItsParentsConnection() { - // Nothing marks a stream child, and its own pipeline carries no multiplex handler either, so the two - // agree here as well: a stream is not the connection that multiplexes it. + void aStreamOfAnHttp2ConnectionIsNotTheConnection() { + // A real stream child rather than a bare channel: what is worth pinning is that a stream does not + // inherit the connection state its parent carries, since that is now what identifies an HTTP/2 + // connection. The stream is where a request is written, so mistaking it for its parent would loop. channelManager.upgradePipelineToHttp2(channel.pipeline()); - EmbeddedChannel stream = new EmbeddedChannel(); + channel.runPendingTasks(); + + Channel stream = new Http2StreamChannelBootstrap(channel) + .handler(new ChannelInboundHandlerAdapter()) + .open().syncUninterruptibly().getNow(); try { - assertNull(stream.pipeline().get(ChannelManager.HTTP2_MULTIPLEX)); - assertFalse(ChannelManager.isHttp2(stream)); + assertNull(stream.pipeline().get(ChannelManager.HTTP2_MULTIPLEX), + "a stream child carries no multiplex handler of its own"); + assertFalse(ChannelManager.isHttp2(stream), "and is not the connection that multiplexes it"); } finally { - stream.finishAndReleaseAll(); + stream.close().syncUninterruptibly(); } } }