Do not use hard-coded handler names in HTTP/2

Motivation:

Our HTTP/2 implementation sometimes uses hard-coded handler names when
adding/removing a handler to/from a pipeline. It's not really a good
idea because it can easily result in name clashes. Unless there is a
good reason, we need to use the reference to the handlers

Modifications:

- Allow null as a handler name for Http2Client/ServerUpgradeCodec
  - Use null as the default upgrade handler name
- Do not use handler name strings in some test cases and examples

Result:

Fixes netty#3815

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java

@@ -14,16 +14,6 @@
  */
 package io.netty.handler.codec.http2;
 
-import static io.netty.handler.codec.base64.Base64Dialect.URL_SAFE;
-import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_PROTOCOL_NAME;
-import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_SETTINGS_HEADER;
-import static io.netty.handler.codec.http2.Http2CodecUtil.SETTING_ENTRY_LENGTH;
-import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedInt;
-import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedShort;
-import static io.netty.util.CharsetUtil.UTF_8;
-import static io.netty.util.ReferenceCountUtil.release;
-import static io.netty.util.internal.ObjectUtil.checkNotNull;
-
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.base64.Base64;
@@ -36,6 +26,16 @@
 import java.util.Collections;
 import java.util.List;
 
+import static io.netty.handler.codec.base64.Base64Dialect.URL_SAFE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_PROTOCOL_NAME;
+import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_SETTINGS_HEADER;
+import static io.netty.handler.codec.http2.Http2CodecUtil.SETTING_ENTRY_LENGTH;
+import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedInt;
+import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedShort;
+import static io.netty.util.CharsetUtil.UTF_8;
+import static io.netty.util.ReferenceCountUtil.release;
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
 /**
  * Client-side cleartext upgrade codec from HTTP to HTTP/2.
  */
@@ -50,21 +50,21 @@ public class Http2ClientUpgradeCodec implements HttpClientUpgradeHandler.Upgrade
      * Creates the codec using a default name for the connection handler when adding to the
      * pipeline.
      *
-     * @param connectionHandler the HTTP/2 connection handler.
+     * @param connectionHandler the HTTP/2 connection handler
      */
     public Http2ClientUpgradeCodec(Http2ConnectionHandler connectionHandler) {
-        this("http2ConnectionHandler", connectionHandler);
+        this(null, connectionHandler);
     }
 
     /**
      * 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.
-     * @param connectionHandler the HTTP/2 connection handler.
+     * @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) {
-        this.handlerName = checkNotNull(handlerName, "handlerName");
+    public Http2ClientUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler) {
+        this.handlerName = handlerName;
         this.connectionHandler = checkNotNull(connectionHandler, "connectionHandler");
     }
 

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java

@@ -14,14 +14,6 @@
  */
 package io.netty.handler.codec.http2;
 
-import static io.netty.handler.codec.base64.Base64Dialect.URL_SAFE;
-import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
-import static io.netty.handler.codec.http2.Http2CodecUtil.FRAME_HEADER_LENGTH;
-import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_PROTOCOL_NAME;
-import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_SETTINGS_HEADER;
-import static io.netty.handler.codec.http2.Http2CodecUtil.writeFrameHeader;
-import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS;
-import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
 import io.netty.channel.ChannelHandlerContext;
@@ -36,6 +28,15 @@
 import java.util.Collections;
 import java.util.List;
 
+import static io.netty.handler.codec.base64.Base64Dialect.URL_SAFE;
+import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
+import static io.netty.handler.codec.http2.Http2CodecUtil.FRAME_HEADER_LENGTH;
+import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_PROTOCOL_NAME;
+import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_SETTINGS_HEADER;
+import static io.netty.handler.codec.http2.Http2CodecUtil.writeFrameHeader;
+import static io.netty.handler.codec.http2.Http2FrameTypes.SETTINGS;
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
 /**
  * Server-side codec for performing a cleartext upgrade from HTTP/1.x to HTTP/2.
  */
@@ -52,20 +53,21 @@ public class Http2ServerUpgradeCodec implements HttpServerUpgradeHandler.Upgrade
      * Creates the codec using a default name for the connection handler when adding to the
      * pipeline.
      *
-     * @param connectionHandler the HTTP/2 connection handler.
+     * @param connectionHandler the HTTP/2 connection handler
      */
     public Http2ServerUpgradeCodec(Http2ConnectionHandler connectionHandler) {
-        this("http2ConnectionHandler", connectionHandler);
+        this(null, connectionHandler);
     }
 
     /**
      * 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.
-     * @param connectionHandler the HTTP/2 connection handler.
+     * @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 Http2ServerUpgradeCodec(String handlerName, Http2ConnectionHandler connectionHandler) {
-        this.handlerName = checkNotNull(handlerName, "handlerName");
+        this.handlerName = handlerName;
         this.connectionHandler = checkNotNull(connectionHandler, "connectionHandler");
         frameReader = new DefaultHttp2FrameReader();
     }

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameRoundtripTest.java

@@ -15,19 +15,6 @@
 
 package io.netty.handler.codec.http2;
 
-import static io.netty.handler.codec.http2.Http2TestUtil.as;
-import static io.netty.handler.codec.http2.Http2TestUtil.randomString;
-import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
-import static io.netty.util.CharsetUtil.UTF_8;
-import static java.util.concurrent.TimeUnit.SECONDS;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyInt;
-import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
 import io.netty.bootstrap.Bootstrap;
 import io.netty.bootstrap.ServerBootstrap;
 import io.netty.buffer.ByteBuf;
@@ -44,6 +31,13 @@
 import io.netty.handler.codec.http2.Http2TestUtil.Http2Runnable;
 import io.netty.util.NetUtil;
 import io.netty.util.concurrent.Future;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
 
 import java.net.InetSocketAddress;
 import java.util.ArrayList;
@@ -52,13 +46,19 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
-import org.mockito.invocation.InvocationOnMock;
-import org.mockito.stubbing.Answer;
+import static io.netty.handler.codec.http2.Http2TestUtil.as;
+import static io.netty.handler.codec.http2.Http2TestUtil.randomString;
+import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
+import static io.netty.util.CharsetUtil.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 /**
  * Tests encoding/decoding each HTTP2 frame type.
@@ -362,7 +362,7 @@ private void bootstrapEnv(int requestCountDown) throws Exception {
             protected void initChannel(Channel ch) throws Exception {
                 ChannelPipeline p = ch.pipeline();
                 serverAdapter = new Http2TestUtil.FrameAdapter(serverListener, requestLatch);
-                p.addLast("reader", serverAdapter);
+                p.addLast(serverAdapter);
             }
         });
 
@@ -372,7 +372,7 @@ protected void initChannel(Channel ch) throws Exception {
             @Override
             protected void initChannel(Channel ch) throws Exception {
                 ChannelPipeline p = ch.pipeline();
-                p.addLast("reader", new Http2TestUtil.FrameAdapter(null, null));
+                p.addLast(new Http2TestUtil.FrameAdapter(null, null));
             }
         });
 

codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java

@@ -14,17 +14,6 @@
  */
 package io.netty.handler.codec.http2;
 
-import static io.netty.handler.codec.http2.Http2Exception.isStreamError;
-import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception;
-import static io.netty.handler.codec.http2.Http2TestUtil.as;
-import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
-import static java.util.concurrent.TimeUnit.SECONDS;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.reset;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
 import io.netty.bootstrap.Bootstrap;
 import io.netty.bootstrap.ServerBootstrap;
 import io.netty.buffer.ByteBuf;
@@ -57,18 +46,29 @@
 import io.netty.util.CharsetUtil;
 import io.netty.util.NetUtil;
 import io.netty.util.concurrent.Future;
-
-import java.net.InetSocketAddress;
-import java.util.List;
-import java.util.concurrent.CountDownLatch;
-
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import java.net.InetSocketAddress;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+
+import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception;
+import static io.netty.handler.codec.http2.Http2Exception.isStreamError;
+import static io.netty.handler.codec.http2.Http2TestUtil.as;
+import static io.netty.handler.codec.http2.Http2TestUtil.runInChannel;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
 /**
  * Testing the {@link InboundHttp2ToHttpPriorityAdapter} and base class {@link InboundHttp2ToHttpAdapter} for HTTP/2
  * frames into {@link HttpObject}s
@@ -124,15 +124,16 @@ public void setup() throws Exception {
             protected void initChannel(Channel ch) throws Exception {
                 ChannelPipeline p = ch.pipeline();
                 Http2Connection connection = new DefaultHttp2Connection(true);
-                p.addLast(
-                        "reader",
-                        new HttpAdapterFrameAdapter(connection,
-                                new InboundHttp2ToHttpPriorityAdapter.Builder(connection)
-                                        .maxContentLength(maxContentLength)
-                                        .validateHttpHeaders(true)
-                                        .propagateSettings(true)
-                                        .build(),
-                                        new CountDownLatch(10)));
+
+                p.addLast(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;
@@ -160,13 +161,14 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
             protected void initChannel(Channel ch) throws Exception {
                 ChannelPipeline p = ch.pipeline();
                 Http2Connection connection = new DefaultHttp2Connection(false);
-                p.addLast(
-                        "reader",
-                        new HttpAdapterFrameAdapter(connection,
-                                new InboundHttp2ToHttpPriorityAdapter.Builder(connection)
+
+                p.addLast(new HttpAdapterFrameAdapter(
+                        connection,
+                        new InboundHttp2ToHttpPriorityAdapter.Builder(connection)
                                 .maxContentLength(maxContentLength)
                                 .build(),
-                                new CountDownLatch(10)));
+                        new CountDownLatch(10)));
+
                 clientDelegator = new HttpResponseDelegator(clientListener, clientLatch);
                 p.addLast(clientDelegator);
             }

example/src/main/java/io/netty/example/http2/helloworld/client/Http2ClientInitializer.java

@@ -14,8 +14,6 @@
  */
 package io.netty.example.http2.helloworld.client;
 
-import static io.netty.handler.logging.LogLevel.INFO;
-
 import io.netty.channel.ChannelHandlerAdapter;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInitializer;
@@ -41,6 +39,8 @@
 import io.netty.handler.codec.http2.InboundHttp2ToHttpAdapter;
 import io.netty.handler.ssl.SslContext;
 
+import static io.netty.handler.logging.LogLevel.INFO;
+
 /**
  * Configures the client pipeline to support HTTP/2 frames.
  */
@@ -88,17 +88,16 @@ public Http2SettingsHandler settingsHandler() {
     }
 
     protected void configureEndOfPipeline(ChannelPipeline pipeline) {
-        pipeline.addLast("Http2SettingsHandler", settingsHandler);
-        pipeline.addLast("HttpResponseHandler", responseHandler);
+        pipeline.addLast(settingsHandler, responseHandler);
     }
 
     /**
      * Configure the pipeline for TLS NPN negotiation to HTTP/2.
      */
     private void configureSsl(SocketChannel ch) {
         ChannelPipeline pipeline = ch.pipeline();
-        pipeline.addLast("SslHandler", sslCtx.newHandler(ch.alloc()));
-        pipeline.addLast("Http2Handler", connectionHandler);
+        pipeline.addLast(sslCtx.newHandler(ch.alloc()),
+                         connectionHandler);
         configureEndOfPipeline(pipeline);
     }
 
@@ -110,10 +109,10 @@ private void configureClearText(SocketChannel ch) {
         Http2ClientUpgradeCodec upgradeCodec = new Http2ClientUpgradeCodec(connectionHandler);
         HttpClientUpgradeHandler upgradeHandler = new HttpClientUpgradeHandler(sourceCodec, upgradeCodec, 65536);
 
-        ch.pipeline().addLast("Http2SourceCodec", sourceCodec);
-        ch.pipeline().addLast("Http2UpgradeHandler", upgradeHandler);
-        ch.pipeline().addLast("Http2UpgradeRequestHandler", new UpgradeRequestHandler());
-        ch.pipeline().addLast("Logger", new UserEventLogger());
+        ch.pipeline().addLast(sourceCodec,
+                              upgradeHandler,
+                              new UpgradeRequestHandler(),
+                              new UserEventLogger());
     }
 
     /**
@@ -131,7 +130,7 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {
             // Done with this handler, remove it from the pipeline.
             ctx.pipeline().remove(this);
 
-            Http2ClientInitializer.this.configureEndOfPipeline(ctx.pipeline());
+            configureEndOfPipeline(ctx.pipeline());
         }
     }
 

example/src/main/java/io/netty/example/http2/tiles/HttpServer.java

@@ -54,11 +54,10 @@ public ChannelFuture start() throws Exception {
         .childHandler(new ChannelInitializer<SocketChannel>() {
             @Override
             protected void initChannel(SocketChannel ch) throws Exception {
-                ChannelPipeline pipeline = ch.pipeline();
-                pipeline.addLast("httpRequestDecoder", new HttpRequestDecoder());
-                pipeline.addLast("httpResponseEncoder", new HttpResponseEncoder());
-                pipeline.addLast("httpChunkAggregator", new HttpObjectAggregator(MAX_CONTENT_LENGTH));
-                pipeline.addLast("httpRequestHandler", new Http1RequestHandler());
+                ch.pipeline().addLast(new HttpRequestDecoder(),
+                                      new HttpResponseEncoder(),
+                                      new HttpObjectAggregator(MAX_CONTENT_LENGTH),
+                                      new Http1RequestHandler());
             }
         });