Use the HttpServerResponse to send a response on an handshake failure…

… so the HTTP connection remains usable after - fixes #2878

src/main/java/io/vertx/core/http/HttpServerRequest.java

@@ -300,10 +300,12 @@ default HttpServerRequest bodyHandler(@Nullable Handler<Buffer> bodyHandler) {
   /**
    * Upgrade the connection to a WebSocket connection.
    * <p>
-   * This is an alternative way of handling WebSockets and can only be used if no websocket handlers are set on the
-   * Http server, and can only be used during the upgrade request during the WebSocket handshake.
+   * This is an alternative way of handling WebSockets and can only be used if no WebSocket handler is set on the
+   * {@code HttpServer}, and can only be used during the upgrade request during the WebSocket handshake.
    *
-   * @return  the WebSocket
+   * @return the WebSocket
+   * @throws IllegalStateException if the current request cannot be upgraded, when it happens an appropriate response
+   *                               is sent
    */
   ServerWebSocket upgrade();
 

src/main/java/io/vertx/core/http/impl/Http1xServerConnection.java

@@ -43,6 +43,7 @@
 import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
 import static io.netty.handler.codec.http.HttpResponseStatus.CONTINUE;
 import static io.netty.handler.codec.http.HttpResponseStatus.METHOD_NOT_ALLOWED;
+import static io.netty.handler.codec.http.HttpResponseStatus.UPGRADE_REQUIRED;
 import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
 import static io.vertx.core.spi.metrics.Metrics.METRICS_ENABLED;
 
@@ -319,11 +320,10 @@ ServerWebSocketImpl createWebSocket(HttpServerRequestImpl request) {
     if (ws != null) {
       return ws;
     }
-    if (!(request.getRequest() instanceof FullHttpRequest)) {
+    if (!(request.nettyRequest() instanceof FullHttpRequest)) {
       throw new IllegalStateException();
     }
-    FullHttpRequest nettyReq = (FullHttpRequest) request.getRequest();
-    WebSocketServerHandshaker handshaker = createHandshaker(nettyReq);
+    WebSocketServerHandshaker handshaker = createHandshaker(request);
     if (handshaker == null) {
       return null;
     }
@@ -335,25 +335,31 @@ ServerWebSocketImpl createWebSocket(HttpServerRequestImpl request) {
     return ws;
   }
 
-  private WebSocketServerHandshaker createHandshaker(HttpRequest request) {
+  private WebSocketServerHandshaker createHandshaker(HttpServerRequestImpl request) {
     // As a fun part, Firefox 6.0.2 supports Websockets protocol '7'. But,
     // it doesn't send a normal 'Connection: Upgrade' header. Instead it
     // sends: 'Connection: keep-alive, Upgrade'. Brilliant.
     Channel ch = channel();
-    String connectionHeader = request.headers().get(io.vertx.core.http.HttpHeaders.CONNECTION);
+    String connectionHeader = request.getHeader(io.vertx.core.http.HttpHeaders.CONNECTION);
     if (connectionHeader == null || !connectionHeader.toLowerCase().contains("upgrade")) {
-      HttpUtils.sendError(ch, BAD_REQUEST, "\"Connection\" header must be \"Upgrade\".");
+      request.response()
+        .setStatusCode(BAD_REQUEST.code())
+        .end("\"Connection\" header must be \"Upgrade\".");
       return null;
     }
-    if (request.method() != HttpMethod.GET) {
-      HttpUtils.sendError(ch, METHOD_NOT_ALLOWED, null);
+    if (request.method() != io.vertx.core.http.HttpMethod.GET) {
+      request.response()
+        .setStatusCode(METHOD_NOT_ALLOWED.code())
+        .end();
       return null;
     }
     String wsURL;
     try {
       wsURL = HttpUtils.getWebSocketLocation(request, isSsl());
     } catch (Exception e) {
-      HttpUtils.sendError(ch, BAD_REQUEST, "Invalid request URI");
+      request.response()
+        .setStatusCode(BAD_REQUEST.code())
+        .end("Invalid request URI");
       return null;
     }
 
@@ -362,10 +368,13 @@ private WebSocketServerHandshaker createHandshaker(HttpRequest request) {
         options.getWebsocketSubProtocols(),
         options.getPerMessageWebsocketCompressionSupported() || options.getPerFrameWebsocketCompressionSupported(),
         options.getMaxWebsocketFrameSize(), options.isAcceptUnmaskedFrames());
-    WebSocketServerHandshaker shake = factory.newHandshaker(request);
+    WebSocketServerHandshaker shake = factory.newHandshaker(request.nettyRequest());
     if (shake == null) {
-      //log.error("Unrecognised websockets handshake");
-      WebSocketServerHandshakerFactory.sendUnsupportedVersionResponse(ch);
+      // See WebSocketServerHandshakerFactory.sendUnsupportedVersionResponse(ch);
+      request.response()
+        .putHeader(HttpHeaderNames.SEC_WEBSOCKET_VERSION, WebSocketVersion.V13.toHttpHeaderValue())
+        .setStatusCode(UPGRADE_REQUIRED.code())
+        .end();
     }
     return shake;
   }

src/main/java/io/vertx/core/http/impl/HttpServerRequestImpl.java

@@ -62,7 +62,7 @@ public class HttpServerRequestImpl implements HttpServerRequest {
   private final Http1xServerConnection conn;
   final ContextInternal context;
 
-  private DefaultHttpRequest request;
+  private HttpRequest request;
   private io.vertx.core.http.HttpVersion version;
   private io.vertx.core.http.HttpMethod method;
   private String rawMethod;
@@ -92,19 +92,23 @@ public class HttpServerRequestImpl implements HttpServerRequest {
 
   private InboundBuffer<Buffer> pending;
 
-  HttpServerRequestImpl(Http1xServerConnection conn, DefaultHttpRequest request) {
+  HttpServerRequestImpl(Http1xServerConnection conn, HttpRequest request) {
     this.conn = conn;
     this.context = conn.getContext().duplicate();
     this.request = request;
   }
 
-  DefaultHttpRequest getRequest() {
+  /**
+   *
+   * @return
+   */
+  HttpRequest nettyRequest() {
     synchronized (conn) {
       return request;
     }
   }
 
-  void setRequest(DefaultHttpRequest request) {
+  void setRequest(HttpRequest request) {
     synchronized (conn) {
       this.request = request;
     }

src/main/java/io/vertx/core/http/impl/HttpServerResponseImpl.java

@@ -16,7 +16,6 @@
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelPromise;
 import io.netty.handler.codec.http.*;
-import io.netty.util.concurrent.GenericFutureListener;
 import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.AsyncResult;
 import io.vertx.core.Future;
@@ -82,7 +81,7 @@ public class HttpServerResponseImpl implements HttpServerResponse {
   private long bytesWritten;
   private NetSocket netSocket;
 
-  HttpServerResponseImpl(final VertxInternal vertx, ContextInternal context, Http1xServerConnection conn, DefaultHttpRequest request, Object requestMetric) {
+  HttpServerResponseImpl(final VertxInternal vertx, ContextInternal context, Http1xServerConnection conn, HttpRequest request, Object requestMetric) {
     this.vertx = vertx;
     this.conn = conn;
     this.context = context;

src/main/java/io/vertx/core/http/impl/HttpUtils.java

@@ -626,7 +626,7 @@ static void sendError(Channel ch, HttpResponseStatus status, CharSequence err) {
     ch.writeAndFlush(resp);
   }
 
-  static String getWebSocketLocation(HttpRequest req, boolean ssl) throws Exception {
+  static String getWebSocketLocation(HttpServerRequest req, boolean ssl) throws Exception {
     String prefix;
     if (ssl) {
       prefix = "ws://";

src/main/java/io/vertx/core/http/impl/ServerWebSocketImpl.java

@@ -14,9 +14,7 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelPipeline;
-import io.netty.handler.codec.http.FullHttpRequest;
 import io.netty.handler.codec.http.HttpResponseStatus;
-import io.netty.handler.codec.http.websocketx.WebSocketHandshakeException;
 import io.netty.handler.codec.http.websocketx.WebSocketServerHandshaker;
 import io.vertx.core.Future;
 import io.vertx.core.MultiMap;
@@ -29,8 +27,6 @@
 import javax.net.ssl.SSLSession;
 import javax.security.cert.X509Certificate;
 
-import java.util.function.Function;
-
 import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
 
 import static io.netty.handler.codec.http.HttpResponseStatus.SWITCHING_PROTOCOLS;
@@ -174,10 +170,9 @@ private void handleHandshake(int sc) {
   private void doHandshake() {
     Channel channel = conn.channel();
     try {
-      handshaker.handshake(channel, request.getRequest());
+      handshaker.handshake(channel, request.nettyRequest());
     } catch (Exception e) {
-      status = BAD_REQUEST.code();
-      HttpUtils.sendError(conn.channel(), BAD_REQUEST, "\"Connection\" header must be \"Upgrade\".");
+      request.response().setStatusCode(BAD_REQUEST.code()).end();
       throw e;
     } finally {
       request = null;

src/main/java/io/vertx/core/http/impl/WebSocketRequestHandler.java

@@ -13,12 +13,12 @@
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
 import io.netty.handler.codec.http.DefaultHttpRequest;
 import io.netty.handler.codec.http.EmptyHttpHeaders;
+import io.netty.handler.codec.http.HttpRequest;
 import io.vertx.core.Handler;
 import io.vertx.core.buffer.Buffer;
 import io.vertx.core.http.HttpServerRequest;
 import io.vertx.core.spi.metrics.HttpServerMetrics;
 
-import static io.netty.handler.codec.http.HttpResponseStatus.SWITCHING_PROTOCOLS;
 import static io.vertx.core.http.HttpHeaders.UPGRADE;
 import static io.vertx.core.http.HttpHeaders.WEBSOCKET;
 import static io.vertx.core.http.impl.HttpUtils.SC_SWITCHING_PROTOCOLS;
@@ -80,7 +80,7 @@ private void handle(HttpServerRequestImpl req) {
    * Handle the request once we have the full body.
    */
   private void handle(HttpServerRequestImpl req, Buffer body) {
-    DefaultHttpRequest nettyReq = req.getRequest();
+    HttpRequest nettyReq = req.nettyRequest();
     nettyReq = new DefaultFullHttpRequest(
       nettyReq.protocolVersion(),
       nettyReq.method(),

src/test/java/io/vertx/core/http/WebSocketTest.java

@@ -60,7 +60,9 @@
 import java.util.function.Consumer;
 import java.util.function.Function;
 
-import static io.vertx.core.http.HttpTestBase.*;
+import static io.vertx.core.http.HttpTestBase.DEFAULT_HTTP_HOST;
+import static io.vertx.core.http.HttpTestBase.DEFAULT_HTTP_PORT;
+import static io.vertx.core.http.HttpTestBase.DEFAULT_TEST_URI;
 import static io.vertx.test.core.TestUtils.*;
 
 /**
@@ -1202,26 +1204,42 @@ private void testInvalidHandshake(Function<Handler<AsyncResult<HttpClientRespons
                                     boolean expectEvent,
                                     boolean upgradeRequest,
                                     int expectedStatus) {
+    client.close();
+    client = vertx.createHttpClient(new HttpClientOptions().setMaxPoolSize(1));
     if (upgradeRequest) {
-      server = vertx.createHttpServer().websocketHandler(ws -> {
-        // Check we can get notified
-        // handshake fails after this method returns and does not reject the ws
-        assertTrue(expectEvent);
-      });
+      server = vertx.createHttpServer()
+        .websocketHandler(ws -> {
+          // Check we can get notified
+          // handshake fails after this method returns and does not reject the ws
+          assertTrue(expectEvent);
+        })
+        .requestHandler(req -> {
+          req.response().end();
+        });
     } else {
+      AtomicBoolean first = new AtomicBoolean();
       server = vertx.createHttpServer().requestHandler(req -> {
-        try {
-          req.upgrade();
-        } catch (Exception e) {
-          // Expected
+        if (first.compareAndSet(false, true)) {
+          try {
+            req.upgrade();
+          } catch (Exception e) {
+            // Expected
+          }
+        } else {
+          req.response().end();
         }
       });
     }
     server.listen(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, onSuccess(ar -> {
       HttpClientRequest req = requestProvider.apply(onSuccess(resp -> {
         assertEquals(expectedStatus, resp.statusCode());
         resp.endHandler(v1 -> {
-          testComplete();
+          // Make another request to check the connection remains usable
+          client.getNow(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, onSuccess(resp2 -> {
+            resp2.endHandler(v2 -> {
+              testComplete();
+            });
+          }));
         });
       }));
       req.end();