From 4d6ab1d30de2fd06bc7544ce6301fd6b85d183d7 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Fri, 29 Jan 2016 11:59:51 +0900 Subject: [PATCH] Fix missing trailing data on HTTP client upgrade Motivation: When HttpClientUpgradeHandler upgrades from HTTP/1 to another protocol, it performs a two-step opertion: 1. Remove the SourceCodec (HttpClientCodec) 2. Add the UpgradeCodec When HttpClientCodec is removed from the pipeline, the decoder being removed triggers channelRead() event with the data left in its cumulation buffer. However, this is not received by the UpgradeCodec becuase it's not added yet. e.g. HTTP/2 SETTINGS frame sent by the server can be missed out. To fix the problem, we need to reverse the steps: 1. Add the UpgradeCodec 2. Remove the SourceCodec However, this does not work as expected either, because UpgradeCodec can send a greeting message such as HTTP/2 Preface. Such a greeting message will be handled by the SourceCodec and will trigger an 'unsupported message type' exception. To fix the problem really, we need to make the upgrade process 3-step: 1. Remove/disable the encoder of SourceCodec 2. Add the UpgradeCodec 3. Remove the SourceCodec Modifications: - Add SourceCodec.prepareUpgradeFrom() so that SourceCodec can remove or disable its encoder - Implement HttpClientCodec.prepareUpgradeFrom() properly - Miscellaneous: - Log the related channel as well When logging the failure to send a GOAWAY Result: Cleartext HTTP/1-to-HTTP/2 upgrade works again. --- .../handler/codec/http/HttpClientCodec.java | 17 +++++++++++++++++ .../codec/http/HttpClientUpgradeHandler.java | 10 +++++++++- .../codec/http2/Http2ConnectionHandler.java | 16 ++++++---------- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java index bc0680bea7..038e180259 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java @@ -21,6 +21,7 @@ import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelPipeline; import io.netty.channel.CombinedChannelDuplexHandler; import io.netty.handler.codec.PrematureChannelClosureException; +import io.netty.util.ReferenceCountUtil; import java.util.ArrayDeque; import java.util.List; @@ -87,6 +88,14 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler out) throws Exception { + + if (upgraded) { + out.add(ReferenceCountUtil.retain(msg)); + return; + } + if (msg instanceof HttpRequest && !done) { queue.offer(((HttpRequest) msg).method()); } diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java index 1d4d74bb12..9fdf4afb85 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientUpgradeHandler.java @@ -62,6 +62,13 @@ public class HttpClientUpgradeHandler extends HttpObjectAggregator implements Ch * The source codec that is used in the pipeline initially. */ public interface SourceCodec { + + /** + * Removes or disables the encoder of this codec so that the {@link UpgradeCodec} can send an initial greeting + * (if any). + */ + void prepareUpgradeFrom(ChannelHandlerContext ctx); + /** * Removes this codec (i.e. all associated handlers) from the pipeline. */ @@ -222,8 +229,9 @@ public class HttpClientUpgradeHandler extends HttpObjectAggregator implements Ch } // Upgrade to the new protocol. - sourceCodec.upgradeFrom(ctx); + sourceCodec.prepareUpgradeFrom(ctx); upgradeCodec.upgradeTo(ctx, response); + sourceCodec.upgradeFrom(ctx); // Notify that the upgrade to the new protocol completed successfully. ctx.fireUserEventTriggered(UpgradeEvent.UPGRADE_SUCCESSFUL); diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java index 134b1a05f9..28c1804bfd 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java @@ -47,7 +47,6 @@ import static io.netty.handler.codec.http2.Http2Stream.State.IDLE; import static io.netty.util.CharsetUtil.UTF_8; import static io.netty.util.internal.ObjectUtil.checkNotNull; import static java.lang.Math.min; -import static java.lang.String.format; import static java.util.concurrent.TimeUnit.MILLISECONDS; /** @@ -724,20 +723,17 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http if (future.isSuccess()) { if (errorCode != NO_ERROR.code()) { if (logger.isDebugEnabled()) { - logger.debug( - format("Sent GOAWAY: lastStreamId '%d', errorCode '%d', " + - "debugData '%s'. Forcing shutdown of the connection.", - lastStreamId, errorCode, debugData.toString(UTF_8)), - future.cause()); + logger.debug("{} Sent GOAWAY: lastStreamId '{}', errorCode '{}', " + + "debugData '{}'. Forcing shutdown of the connection.", + ctx.channel(), lastStreamId, errorCode, debugData.toString(UTF_8), future.cause()); } ctx.close(); } } else { if (logger.isErrorEnabled()) { - logger.error( - format("Sending GOAWAY failed: lastStreamId '%d', errorCode '%d', " + - "debugData '%s'. Forcing shutdown of the connection.", - lastStreamId, errorCode, debugData.toString(UTF_8)), future.cause()); + logger.error("{} Sending GOAWAY failed: lastStreamId '{}', errorCode '{}', " + + "debugData '{}'. Forcing shutdown of the connection.", + ctx.channel(), lastStreamId, errorCode, debugData.toString(UTF_8), future.cause()); } ctx.close(); }