Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Boolean> HTTP2_CONNECTION_ATTRIBUTE = AttributeKey.valueOf("http2Connection");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttributeKey.valueOf(String) lands in the global constant pool, so any other code in the JVM that uses "http2Connection" ends up with the same key. SuspensionAwareHttp2LocalFlowController already uses the class scoped form, can we do the same here ?

Suggested change
private static final AttributeKey<Boolean> HTTP2_CONNECTION_ATTRIBUTE = AttributeKey.valueOf("http2Connection");
private static final AttributeKey<Boolean> HTTP2_CONNECTION_ATTRIBUTE = AttributeKey.valueOf(ChannelManager.class, "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);
Expand Down Expand Up @@ -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.
* <p>
* 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasAttr is true as soon as an entry exists for the key, not when the value is TRUE. A plain channel.attr(HTTP2_CONNECTION_ATTRIBUTE) read is enough to make this say HTTP/2 for an HTTP/1.1 channel, and a later set(false) would not undo it. attr(...).get() is the idiom used everywhere else in this class, so I think we hit this at some point.

Suggested change
return channel.hasAttr(HTTP2_CONNECTION_ATTRIBUTE);
return Boolean.TRUE.equals(channel.attr(HTTP2_CONNECTION_ATTRIBUTE).get());

}

/**
Expand Down Expand Up @@ -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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set HTTP2_STATE_KEY on this same channel three lines below, and writeHttp2Request reads it back as its first statement. Why not route on that and drop the new key ? writeRequest does one attr(HTTP2_STATE_KEY).get() and passes the state in, which loses the pipeline walk, the second lookup inside writeHttp2Request and the second marker to keep in sync.


// Attach HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining
Http2ConnectionState state = new Http2ConnectionState();
Expand Down
Original file line number Diff line number Diff line change
@@ -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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nothing here needs a ChannelManager per test, the state under test lives on the EmbeddedChannel. @BeforeAll with a close in @AfterAll takes the class from about 8s to roughly a third of that. The sibling ChannelManager tests run in under 0.2s.

channel = new EmbeddedChannel();
}

@AfterEach
void tearDown() {
channel.finishAndReleaseAll();
timer.stop();
Comment on lines +51 to +52

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

channelManager.close() is missing. The constructor builds an SslContext and an event loop group per test, and ChannelManagerHttp2DrainPermitTest, ChannelManagerHttp2SiblingLookupTest and NettyConnectListenerPermitLeakTest all close it in @AfterEach. I replayed this setUp and tearDown three times and the fork gained about 6 fds each round, never given back. Worth guarding for null too, like the sibling tests do, otherwise a failure in setUp gets reported as an NPE from here.

Suggested change
channel.finishAndReleaseAll();
timer.stop();
if (channel != null) {
channel.finishAndReleaseAll();
}
if (channelManager != null) {
channelManager.close();
}
if (timer != null) {
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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a plain EmbeddedChannel with no parent, so the test asserts the same thing as aConnectionThatWasNeverUpgradedIsNotHttp2. The case worth pinning is a real stream child not picking up the parent attribute, which is new with this change:

channel.runPendingTasks();
Channel stream = new Http2StreamChannelBootstrap(channel)
        .handler(new ChannelInboundHandlerAdapter())
        .open().syncUninterruptibly().getNow();

That gives a real Http2StreamChannel and isHttp2 on it is false, so the assert still holds, and it would break if a Netty version ever made attr delegate to the parent.

try {
assertNull(stream.pipeline().get(ChannelManager.HTTP2_MULTIPLEX));
assertFalse(ChannelManager.isHttp2(stream));
} finally {
stream.finishAndReleaseAll();
}
}
}
Loading