Repository: netty/netty
Issue: Remove misleading argument from HttpServerUpgradeHandler.UpgradeCodec.upgradeTo
Labels: defect

`upgradeTo()` is currently passed `upgradeResponse`, but it doesn't appear like it can do anything useful with it since the response has already been sent. In addition, the [`upgradeTo()` documentation](https://github.com/netty/netty/blob/43ebbc3fa065155fa67732b0cbd7c12843b0f3f7/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java#L75) is sort of misleading as it seems to imply that it should be sent and may be modified before sending.


Issue comments:
@ejona86 agree... let me take care of it.

Fixed by https://github.com/netty/netty/pull/4989


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
index 691d37e7675..1c9f8cda850 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java
@@ -70,14 +70,9 @@ void prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest upgradeRe
          * adding all handlers required for the new protocol.
          *
          * @param ctx the context for the current handler.
-         * @param upgradeRequest the request that triggered the upgrade to this protocol. The
-         *            upgraded protocol is responsible for sending the response.
-         * @param upgradeResponse a 101 Switching Protocols response that is populated with the
-         *            {@link HttpHeaderNames#CONNECTION} and {@link HttpHeaderNames#UPGRADE} headers.
-         *            The protocol is required to send this before sending any other frames back to the client.
-         *            The headers may be augmented as necessary by the protocol before sending.
+         * @param upgradeRequest the request that triggered the upgrade to this protocol.
          */
-        void upgradeTo(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest, FullHttpResponse upgradeResponse);
+        void upgradeTo(ChannelHandlerContext ctx, FullHttpRequest upgradeRequest);
     }
 
     /**
@@ -320,7 +315,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
                     if (future.isSuccess()) {
                         // Perform the upgrade to the new protocol.
                         sourceCodec.upgradeFrom(ctx);
-                        finalUpgradeCodec.upgradeTo(ctx, request, upgradeResponse);
+                        finalUpgradeCodec.upgradeTo(ctx, request);
 
                         // Notify that the upgrade has occurred. Retain the event to offset
                         // the release() in the finally block.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
index fa00076ad71..3382fc29385 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java
@@ -98,8 +98,7 @@ public void prepareUpgradeResponse(ChannelHandlerContext ctx, FullHttpRequest up
     }
 
     @Override
-    public void upgradeTo(final ChannelHandlerContext ctx, FullHttpRequest upgradeRequest,
-            FullHttpResponse upgradeResponse) {
+    public void upgradeTo(final ChannelHandlerContext ctx, FullHttpRequest upgradeRequest) {
         // Add the HTTP/2 connection handler to the pipeline immediately following the current handler.
         ctx.pipeline().addAfter(ctx.name(), handlerName, connectionHandler);
     }


Test patch: