Correctly handle client side http2 upgrades when Http2FrameCodec …(9495) (#9501)
Motivation: In the release (4.1.37) we introduced Http2MultiplexHandler as a replacement of Http2MultiplexCodec. This did split the frame parsing from the multiplexing to allow a more flexible way to handle frames and to make the code cleaner. Unfortunally we did miss to special handle this in Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...). This did lead to the situation that we did not correctly receive the event on the Http2MultiplexHandler and so did not correctly created the Http2StreamChannel for the upgrade stream. Because of this we ended up with an NPE if a frame was dispatched to the upgrade stream later on. Modifications: - Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...) Result: Fixes #9495.
This commit is contained in:
parent
b1a821e930
commit
130522be84
@ -47,13 +47,14 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade
|
|||||||
private final String handlerName;
|
private final String handlerName;
|
||||||
private final Http2ConnectionHandler connectionHandler;
|
private final Http2ConnectionHandler connectionHandler;
|
||||||
private final ChannelHandler upgradeToHandler;
|
private final ChannelHandler upgradeToHandler;
|
||||||
|
private final ChannelHandler http2MultiplexHandler;
|
||||||
|
|
||||||
public Http2ClientUpgradeCodec(Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) {
|
public Http2ClientUpgradeCodec(Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) {
|
||||||
this(null, frameCodec, upgradeToHandler);
|
this(null, frameCodec, upgradeToHandler);
|
||||||
}
|
}
|
||||||
|
|
||||||
public Http2ClientUpgradeCodec(String handlerName, Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) {
|
public Http2ClientUpgradeCodec(String handlerName, Http2FrameCodec frameCodec, ChannelHandler upgradeToHandler) {
|
||||||
this(handlerName, (Http2ConnectionHandler) frameCodec, upgradeToHandler);
|
this(handlerName, (Http2ConnectionHandler) frameCodec, upgradeToHandler, null);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@ -66,6 +67,18 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade
|
|||||||
this((String) null, connectionHandler);
|
this((String) null, connectionHandler);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Creates the codec using a default name for the connection handler when adding to the
|
||||||
|
* pipeline.
|
||||||
|
*
|
||||||
|
* @param connectionHandler the HTTP/2 connection handler
|
||||||
|
* @param http2MultiplexHandler the Http2 Multiplexer handler to work with Http2FrameCodec
|
||||||
|
*/
|
||||||
|
public Http2ClientUpgradeCodec(Http2ConnectionHandler connectionHandler,
|
||||||
|
Http2MultiplexHandler http2MultiplexHandler) {
|
||||||
|
this((String) null, connectionHandler, http2MultiplexHandler);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Creates the codec providing an upgrade to the given handler for HTTP/2.
|
* Creates the codec providing an upgrade to the given handler for HTTP/2.
|
||||||
*
|
*
|
||||||
@ -74,17 +87,31 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade
|
|||||||
* @param connectionHandler the HTTP/2 connection handler
|
* @param connectionHandler the HTTP/2 connection handler
|
||||||
*/
|
*/
|
||||||
public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler) {
|
public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler) {
|
||||||
this(handlerName, connectionHandler, connectionHandler);
|
this(handlerName, connectionHandler, connectionHandler, null);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Creates the codec providing an upgrade to the given handler for HTTP/2.
|
||||||
|
*
|
||||||
|
* @param handlerName the name of the HTTP/2 connection handler to be used in the pipeline,
|
||||||
|
* or {@code null} to auto-generate the name
|
||||||
|
* @param connectionHandler the HTTP/2 connection handler
|
||||||
|
*/
|
||||||
|
public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler,
|
||||||
|
Http2MultiplexHandler http2MultiplexHandler) {
|
||||||
|
this(handlerName, connectionHandler, connectionHandler, http2MultiplexHandler);
|
||||||
}
|
}
|
||||||
|
|
||||||
private Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler, ChannelHandler
|
private Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler, ChannelHandler
|
||||||
upgradeToHandler) {
|
upgradeToHandler, Http2MultiplexHandler http2MultiplexHandler) {
|
||||||
this.handlerName = handlerName;
|
this.handlerName = handlerName;
|
||||||
this.connectionHandler = requireNonNull(connectionHandler, "connectionHandler");
|
this.connectionHandler = requireNonNull(connectionHandler, "connectionHandler");
|
||||||
this.upgradeToHandler = requireNonNull(upgradeToHandler, "upgradeToHandler");
|
this.upgradeToHandler = requireNonNull(upgradeToHandler, "upgradeToHandler");
|
||||||
|
this.http2MultiplexHandler = http2MultiplexHandler;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
|
|
||||||
public CharSequence protocol() {
|
public CharSequence protocol() {
|
||||||
return HTTP_UPGRADE_PROTOCOL_NAME;
|
return HTTP_UPGRADE_PROTOCOL_NAME;
|
||||||
}
|
}
|
||||||
@ -100,11 +127,23 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade
|
|||||||
@Override
|
@Override
|
||||||
public void upgradeTo(ChannelHandlerContext ctx, FullHttpResponse upgradeResponse)
|
public void upgradeTo(ChannelHandlerContext ctx, FullHttpResponse upgradeResponse)
|
||||||
throws Exception {
|
throws Exception {
|
||||||
|
try {
|
||||||
// Add the handler to the pipeline.
|
// Add the handler to the pipeline.
|
||||||
ctx.pipeline().addAfter(ctx.name(), handlerName, upgradeToHandler);
|
ctx.pipeline().addAfter(ctx.name(), handlerName, upgradeToHandler);
|
||||||
|
|
||||||
|
// Add the Http2 Multiplex handler as this handler handle events produced by the connectionHandler.
|
||||||
|
// See https://github.com/netty/netty/issues/9495
|
||||||
|
if (http2MultiplexHandler != null) {
|
||||||
|
final String name = ctx.pipeline().context(connectionHandler).name();
|
||||||
|
ctx.pipeline().addAfter(name, null, http2MultiplexHandler);
|
||||||
|
}
|
||||||
|
|
||||||
// Reserve local stream 1 for the response.
|
// Reserve local stream 1 for the response.
|
||||||
connectionHandler.onHttpClientUpgrade();
|
connectionHandler.onHttpClientUpgrade();
|
||||||
|
} catch (Http2Exception e) {
|
||||||
|
ctx.fireExceptionCaught(e);
|
||||||
|
ctx.close();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -32,26 +32,42 @@ public class Http2ClientUpgradeCodecTest {
|
|||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testUpgradeToHttp2ConnectionHandler() throws Exception {
|
public void testUpgradeToHttp2ConnectionHandler() throws Exception {
|
||||||
testUpgrade(new Http2ConnectionHandlerBuilder().server(false).frameListener(new Http2FrameAdapter()).build());
|
testUpgrade(new Http2ConnectionHandlerBuilder().server(false).frameListener(
|
||||||
|
new Http2FrameAdapter()).build(), null);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testUpgradeToHttp2FrameCodec() throws Exception {
|
public void testUpgradeToHttp2FrameCodec() throws Exception {
|
||||||
testUpgrade(Http2FrameCodecBuilder.forClient().build());
|
testUpgrade(Http2FrameCodecBuilder.forClient().build(), null);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testUpgradeToHttp2MultiplexCodec() throws Exception {
|
public void testUpgradeToHttp2MultiplexCodec() throws Exception {
|
||||||
testUpgrade(Http2MultiplexCodecBuilder.forClient(new HttpInboundHandler())
|
testUpgrade(Http2MultiplexCodecBuilder.forClient(new HttpInboundHandler())
|
||||||
.withUpgradeStreamHandler(new ChannelHandler() { }).build());
|
.withUpgradeStreamHandler(new ChannelHandler() { }).build(), null);
|
||||||
}
|
}
|
||||||
|
|
||||||
private static void testUpgrade(Http2ConnectionHandler handler) throws Exception {
|
@Test
|
||||||
|
public void testUpgradeToHttp2FrameCodecWithMultiplexer() throws Exception {
|
||||||
|
testUpgrade(Http2FrameCodecBuilder.forClient().build(),
|
||||||
|
new Http2MultiplexHandler(new HttpInboundHandler(), new HttpInboundHandler()));
|
||||||
|
}
|
||||||
|
|
||||||
|
private static void testUpgrade(Http2ConnectionHandler handler, Http2MultiplexHandler multiplexer)
|
||||||
|
throws Exception {
|
||||||
FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*");
|
FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*");
|
||||||
|
|
||||||
EmbeddedChannel channel = new EmbeddedChannel(new ChannelHandler() { });
|
EmbeddedChannel channel = new EmbeddedChannel(new ChannelHandler() { });
|
||||||
ChannelHandlerContext ctx = channel.pipeline().firstContext();
|
ChannelHandlerContext ctx = channel.pipeline().firstContext();
|
||||||
Http2ClientUpgradeCodec codec = new Http2ClientUpgradeCodec("connectionHandler", handler);
|
|
||||||
|
Http2ClientUpgradeCodec codec;
|
||||||
|
|
||||||
|
if (multiplexer == null) {
|
||||||
|
codec = new Http2ClientUpgradeCodec("connectionHandler", handler);
|
||||||
|
} else {
|
||||||
|
codec = new Http2ClientUpgradeCodec("connectionHandler", handler, multiplexer);
|
||||||
|
}
|
||||||
|
|
||||||
codec.setUpgradeHeaders(ctx, request);
|
codec.setUpgradeHeaders(ctx, request);
|
||||||
// Flush the channel to ensure we write out all buffered data
|
// Flush the channel to ensure we write out all buffered data
|
||||||
channel.flush();
|
channel.flush();
|
||||||
@ -59,6 +75,10 @@ public class Http2ClientUpgradeCodecTest {
|
|||||||
codec.upgradeTo(ctx, null);
|
codec.upgradeTo(ctx, null);
|
||||||
assertNotNull(channel.pipeline().get("connectionHandler"));
|
assertNotNull(channel.pipeline().get("connectionHandler"));
|
||||||
|
|
||||||
|
if (multiplexer != null) {
|
||||||
|
assertNotNull(channel.pipeline().get(Http2MultiplexHandler.class));
|
||||||
|
}
|
||||||
|
|
||||||
assertTrue(channel.finishAndReleaseAll());
|
assertTrue(channel.finishAndReleaseAll());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user