From 6a30c9534b942df56b8a54e7fa3721246e58db32 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 13 Nov 2014 08:29:19 +0100 Subject: [PATCH] Adding a propagateSettings flag to InboundHttp2ToHttpAdapter. Motivation: When DefaultHttp2FrameReader has read a settings frame, the settings will be passed along the pipeline. This allows a client to hold off sending data until it has received a settings frame. But for a server it will always have received a settings frame and the usefulness of this forwarding of settings is less useful. This also causes a debug message to be logged on the server side if there is no channel handler to handle the settings: [nioEventLoopGroup-1-1] DEBUG io.netty.channel.DefaultChannelPipeline - Discarded inbound message {INITIAL_WINDOW_SIZE=131072, MAX_FRAME_SIZE=16384} that reached at the tail of the pipeline. Please check your pipeline configuration. Modifications: Added a builder for the InboundHttp2ToHttpAdapter and InboundHttp2PriortyAdapter and a new parameter named 'propagateSettings' to their constructors. Result: It is now possible to control whether settings should be passed along the pipeline or not. --- .../codec/http2/DefaultHttp2FrameReader.java | 2 - .../http2/InboundHttp2ToHttpAdapter.java | 151 ++++++++++-------- .../InboundHttp2ToHttpPriorityAdapter.java | 87 +++------- .../http2/InboundHttp2ToHttpAdapterTest.java | 72 ++++++++- .../http2/client/Http2ClientInitializer.java | 5 +- 5 files changed, 172 insertions(+), 145 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java index c0c81d29d3..16c11c4b82 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java @@ -482,8 +482,6 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize } } listener.onSettingsRead(ctx, settings); - // Provide an interface for non-listeners to capture settings - ctx.fireChannelRead(settings); } } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java index f9b1383fdd..db9b2bc9f4 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java @@ -61,80 +61,83 @@ public class InboundHttp2ToHttpAdapter extends Http2EventAdapter { protected final boolean validateHttpHeaders; private final ImmediateSendDetector sendDetector; protected final IntObjectMap messageMap; + private final boolean propagateSettings; - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - public static InboundHttp2ToHttpAdapter newInstance(Http2Connection connection, int maxContentLength) { - InboundHttp2ToHttpAdapter instance = new InboundHttp2ToHttpAdapter(connection, maxContentLength); - connection.addListener(instance); - return instance; - } + public static class Builder { - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @param validateHttpHeaders - * - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - public static InboundHttp2ToHttpAdapter newInstance(Http2Connection connection, int maxContentLength, - boolean validateHttpHeaders) { - InboundHttp2ToHttpAdapter instance = new InboundHttp2ToHttpAdapter(connection, - maxContentLength, validateHttpHeaders); - connection.addListener(instance); - return instance; - } + private final Http2Connection connection; + private int maxContentLength; + private boolean validateHttpHeaders; + private boolean propagateSettings; - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - protected InboundHttp2ToHttpAdapter(Http2Connection connection, int maxContentLength) { - this(connection, maxContentLength, true); - } - - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @param validateHttpHeaders - * - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - protected InboundHttp2ToHttpAdapter(Http2Connection connection, int maxContentLength, - boolean validateHttpHeaders) { - checkNotNull(connection, "connection"); - if (maxContentLength <= 0) { - throw new IllegalArgumentException("maxContentLength must be a positive integer: " + maxContentLength); + /** + * Creates a new {@link InboundHttp2ToHttpAdapter} builder for the specified {@link Http2Connection}. + * + * @param connection The object which will provide connection notification events for the current connection + */ + public Builder(Http2Connection connection) { + this.connection = connection; } - this.maxContentLength = maxContentLength; - this.validateHttpHeaders = validateHttpHeaders; - this.connection = connection; + + /** + * Specifies the maximum length of the message content. + * + * @param maxContentLength the maximum length of the message content. If the length of the message content + * exceeds this value, a {@link TooLongFrameException} will be raised + * @return {@link Builder} the builder for the {@link InboundHttp2ToHttpAdapter} + */ + public Builder maxContentLength(int maxContentLength) { + this.maxContentLength = maxContentLength; + return this; + } + + /** + * Specifies whether validation of HTTP headers should be performed. + * + * @param validate + * + * @return {@link Builder} the builder for the {@link InboundHttp2ToHttpAdapter} + */ + public Builder validateHttpHeaders(boolean validate) { + validateHttpHeaders = validate; + return this; + } + + /** + * Specifies whether a read settings frame should be propagated alone the channel pipeline. + * + * @param propagate if {@code true} read settings will be passed along the pipeline. This can be useful + * to clients that need hold off sending data until they have received the settings. + * @return {@link Builder} the builder for the {@link InboundHttp2ToHttpAdapter} + */ + public Builder propagateSettings(boolean propagate) { + propagateSettings = propagate; + return this; + } + + /** + * Builds/creates a new {@link InboundHttp2ToHttpAdapter} instance using this builders current settings. + */ + public InboundHttp2ToHttpAdapter build() { + InboundHttp2ToHttpAdapter instance = new InboundHttp2ToHttpAdapter(this); + connection.addListener(instance); + return instance; + } + } + + protected InboundHttp2ToHttpAdapter(Builder builder) { + checkNotNull(builder.connection, "connection"); + if (builder.maxContentLength <= 0) { + throw new IllegalArgumentException("maxContentLength must be a positive integer: " + + builder.maxContentLength); + } + connection = builder.connection; + maxContentLength = builder.maxContentLength; + validateHttpHeaders = builder.validateHttpHeaders; + propagateSettings = builder.propagateSettings; sendDetector = DEFAULT_SEND_DETECTOR; messageMap = new IntObjectHashMap(); } @@ -318,6 +321,14 @@ public class InboundHttp2ToHttpAdapter extends Http2EventAdapter { processHeadersEnd(ctx, promisedStreamId, msg, false); } + @Override + public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings) throws Http2Exception { + if (propagateSettings) { + // Provide an interface for non-listeners to capture settings + ctx.fireChannelRead(settings); + } + } + /** * Allows messages to be sent up the pipeline before the next phase in the * HTTP message flow is detected. diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java index 1c07731a91..44951c45d3 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java @@ -19,7 +19,6 @@ import java.util.Map.Entry; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.AsciiString; import io.netty.handler.codec.TextHeaders.EntryVisitor; -import io.netty.handler.codec.TooLongFrameException; import io.netty.handler.codec.http.DefaultHttpHeaders; import io.netty.handler.codec.http.FullHttpMessage; import io.netty.handler.codec.http.HttpHeaders; @@ -42,75 +41,27 @@ public final class InboundHttp2ToHttpPriorityAdapter extends InboundHttp2ToHttpA HttpUtil.OUT_OF_MESSAGE_SEQUENCE_RETURN_CODE.toString()); private final IntObjectMap outOfMessageFlowHeaders; - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - public static InboundHttp2ToHttpPriorityAdapter newInstance(Http2Connection connection, int maxContentLength) { - InboundHttp2ToHttpPriorityAdapter instance = new InboundHttp2ToHttpPriorityAdapter(connection, - maxContentLength); - connection.addListener(instance); - return instance; + public static final class Builder extends InboundHttp2ToHttpAdapter.Builder { + + /** + * Creates a new {@link InboundHttp2ToHttpPriorityAdapter} builder for the specified {@link Http2Connection}. + * + * @param connection The object which will provide connection notification events for the current connection + */ + public Builder(Http2Connection connection) { + super(connection); + } + + @Override + public InboundHttp2ToHttpPriorityAdapter build() { + final InboundHttp2ToHttpPriorityAdapter instance = new InboundHttp2ToHttpPriorityAdapter(this); + instance.connection.addListener(instance); + return instance; + } } - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @param validateHttpHeaders - *
    - *
  • {@code true} to validate HTTP headers in the http-codec
    • - *
  • {@code false} not to validate HTTP headers in the http-codec
    • - *
- * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - public static InboundHttp2ToHttpPriorityAdapter newInstance(Http2Connection connection, int maxContentLength, - boolean validateHttpHeaders) { - InboundHttp2ToHttpPriorityAdapter instance = new InboundHttp2ToHttpPriorityAdapter(connection, - maxContentLength, validateHttpHeaders); - connection.addListener(instance); - return instance; - } - - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - private InboundHttp2ToHttpPriorityAdapter(Http2Connection connection, int maxContentLength) { - super(connection, maxContentLength); - outOfMessageFlowHeaders = new IntObjectHashMap(); - } - - /** - * Creates a new instance - * - * @param connection The object which will provide connection notification events for the current connection - * @param maxContentLength the maximum length of the message content. If the length of the message content exceeds - * this value, a {@link TooLongFrameException} will be raised. - * @param validateHttpHeaders - *
    - *
  • {@code true} to validate HTTP headers in the http-codec
    • - *
  • {@code false} not to validate HTTP headers in the http-codec
    • - *
- * @throws NullPointerException If {@code connection} is null - * @throws IllegalArgumentException If {@code maxContentLength} is less than or equal to {@code 0} - */ - private InboundHttp2ToHttpPriorityAdapter(Http2Connection connection, int maxContentLength, - boolean validateHttpHeaders) { - super(connection, maxContentLength, validateHttpHeaders); + private InboundHttp2ToHttpPriorityAdapter(Builder builder) { + super(builder); outOfMessageFlowHeaders = new IntObjectHashMap(); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java index a952b4d4e3..7ebbc7622b 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java @@ -82,6 +82,9 @@ public class InboundHttp2ToHttpAdapterTest { @Mock private HttpResponseListener clientListener; + @Mock + private HttpSettingsListener settingsListener; + private Http2FrameWriter frameWriter; private ServerBootstrap sb; private Bootstrap cb; @@ -90,9 +93,11 @@ public class InboundHttp2ToHttpAdapterTest { private Channel clientChannel; private volatile CountDownLatch serverLatch; private volatile CountDownLatch clientLatch; + private volatile CountDownLatch settingsLatch; private int maxContentLength; private HttpResponseDelegator serverDelegator; private HttpResponseDelegator clientDelegator; + private HttpSettingsDelegator settingsDelegator; private Http2Exception serverException; @Before @@ -105,6 +110,7 @@ public class InboundHttp2ToHttpAdapterTest { maxContentLength = 1024; setServerLatch(1); setClientLatch(1); + setSettingsLatch(1); frameWriter = new DefaultHttp2FrameWriter(); sb = new ServerBootstrap(); @@ -119,11 +125,18 @@ public class InboundHttp2ToHttpAdapterTest { Http2Connection connection = new DefaultHttp2Connection(true); p.addLast( "reader", - new HttpAdapterFrameAdapter(connection, InboundHttp2ToHttpPriorityAdapter.newInstance( - connection, maxContentLength), new CountDownLatch(10))); + new HttpAdapterFrameAdapter(connection, + new InboundHttp2ToHttpPriorityAdapter.Builder(connection) + .maxContentLength(maxContentLength) + .validateHttpHeaders(true) + .propagateSettings(true) + .build(), + new CountDownLatch(10))); serverDelegator = new HttpResponseDelegator(serverListener, serverLatch); p.addLast(serverDelegator); serverConnectedChannel = ch; + settingsDelegator = new HttpSettingsDelegator(settingsListener, settingsLatch); + p.addLast(settingsDelegator); p.addLast(new ChannelHandlerAdapter() { @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { @@ -148,8 +161,11 @@ public class InboundHttp2ToHttpAdapterTest { Http2Connection connection = new DefaultHttp2Connection(false); p.addLast( "reader", - new HttpAdapterFrameAdapter(connection, InboundHttp2ToHttpPriorityAdapter.newInstance( - connection, maxContentLength), new CountDownLatch(10))); + new HttpAdapterFrameAdapter(connection, + new InboundHttp2ToHttpPriorityAdapter.Builder(connection) + .maxContentLength(maxContentLength) + .build(), + new CountDownLatch(10))); clientDelegator = new HttpResponseDelegator(clientListener, clientLatch); p.addLast(clientDelegator); } @@ -663,6 +679,22 @@ public class InboundHttp2ToHttpAdapterTest { } } + @Test + public void propagateSettings() throws Exception { + final Http2Settings settings = new Http2Settings().pushEnabled(true); + runInChannel(clientChannel, new Http2Runnable() { + @Override + public void run() { + frameWriter.writeSettings(ctxClient(), settings, newPromiseClient()); + ctxClient().flush(); + } + }); + assertTrue(settingsLatch.await(3, SECONDS)); + ArgumentCaptor settingsCaptor = ArgumentCaptor.forClass(Http2Settings.class); + verify(settingsListener).messageReceived(settingsCaptor.capture()); + assertEquals(settings, settingsCaptor.getValue()); + } + private void cleanupCapturedRequests() { if (capturedRequests != null) { for (int i = 0; i < capturedRequests.size(); ++i) { @@ -695,6 +727,13 @@ public class InboundHttp2ToHttpAdapterTest { } } + private void setSettingsLatch(int count) { + settingsLatch = new CountDownLatch(count); + if (settingsDelegator != null) { + settingsDelegator.latch(settingsLatch); + } + } + private void awaitRequests() throws Exception { assertTrue(serverLatch.await(2, SECONDS)); } @@ -723,6 +762,10 @@ public class InboundHttp2ToHttpAdapterTest { void messageReceived(HttpObject obj); } + private interface HttpSettingsListener { + void messageReceived(Http2Settings settings); + } + private static final class HttpResponseDelegator extends SimpleChannelInboundHandler { private final HttpResponseListener listener; private volatile CountDownLatch latch; @@ -744,6 +787,27 @@ public class InboundHttp2ToHttpAdapterTest { } } + private static final class HttpSettingsDelegator extends SimpleChannelInboundHandler { + private final HttpSettingsListener listener; + private volatile CountDownLatch latch; + + HttpSettingsDelegator(HttpSettingsListener listener, CountDownLatch latch) { + super(false); + this.listener = listener; + this.latch = latch; + } + + @Override + protected void messageReceived(ChannelHandlerContext ctx, Http2Settings settings) throws Exception { + listener.messageReceived(settings); + latch.countDown(); + } + + public void latch(CountDownLatch latch) { + this.latch = latch; + } + } + private static final class HttpAdapterFrameAdapter extends FrameAdapter { HttpAdapterFrameAdapter(Http2Connection connection, Http2FrameListener listener, CountDownLatch latch) { super(connection, listener, latch); diff --git a/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java b/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java index 3b0eb3fa5c..96ac5b7588 100644 --- a/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java +++ b/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java @@ -71,7 +71,10 @@ public class Http2ClientInitializer extends ChannelInitializer { new DefaultHttp2InboundFlowController(connection, frameWriter), new DefaultHttp2OutboundFlowController(connection, frameWriter), new DelegatingDecompressorFrameListener(connection, - InboundHttp2ToHttpAdapter.newInstance(connection, maxContentLength))); + new InboundHttp2ToHttpAdapter.Builder(connection) + .maxContentLength(maxContentLength) + .propagateSettings(true) + .build())); responseHandler = new HttpResponseHandler(); settingsHandler = new Http2SettingsHandler(ch.newPromise()); if (sslCtx != null) {