From 0fc025d526c66bad385809da11374dab8e90fc51 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 24 Aug 2021 10:37:18 +1000 Subject: [PATCH 1/7] Issue #6642 - add tests for WebSocket Connection Headers. Signed-off-by: Lachlan Roberts --- .../websocket/tests/ConnectionHeaderTest.java | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java diff --git a/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java new file mode 100644 index 000000000000..c0391a3fa4be --- /dev/null +++ b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java @@ -0,0 +1,101 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.websocket.tests; + +import java.net.URI; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jetty.client.HttpRequest; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.client.JettyUpgradeListener; +import org.eclipse.jetty.websocket.client.WebSocketClient; +import org.eclipse.jetty.websocket.server.config.JettyWebSocketServletContainerInitializer; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +public class ConnectionHeaderTest +{ + private static WebSocketClient client; + private static Server server; + private static ServerConnector connector; + + @BeforeAll + public static void startContainers() throws Exception + { + server = new Server(); + connector = new ServerConnector(server); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + JettyWebSocketServletContainerInitializer.configure(contextHandler, (servletContext, container) -> + container.addMapping("/echo", EchoSocket.class)); + + server.setHandler(contextHandler); + server.start(); + + client = new WebSocketClient(); + client.start(); + } + + @AfterAll + public static void stopContainers() throws Exception + { + client.stop(); + server.stop(); + } + + @ParameterizedTest + @ValueSource(strings = {"Upgrade", "keep-alive, Upgrade"}) // TODO: should we test "close, Upgrade", what should we expect? + public void testConnectionKeepAlive(String connectionHeaderValue) throws Exception + { + URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/echo"); + JettyUpgradeListener upgradeListener = new JettyUpgradeListener() + { + @Override + public void onHandshakeRequest(HttpRequest request) + { + HttpFields fields = request.getHeaders(); + if (!(fields instanceof HttpFields.Mutable)) + throw new IllegalStateException(fields.getClass().getName()); + + // Replace the default connection header value with a custom one. + HttpFields.Mutable headers = (HttpFields.Mutable)fields; + headers.put(HttpHeader.CONNECTION, connectionHeaderValue); + } + }; + + EventSocket clientEndpoint = new EventSocket(); + try (Session session = client.connect(clientEndpoint, uri, null, upgradeListener).get(5, TimeUnit.SECONDS)) + { + // Generate text frame + String msg = "this is an echo ... cho ... ho ... o"; + session.getRemote().sendString(msg); + + // Read frame (hopefully text frame) + String response = clientEndpoint.textMessages.poll(5, TimeUnit.SECONDS); + assertThat("Text Frame.status code", response, is(msg)); + } + } +} \ No newline at end of file From f7d9d8fcf887d40b2aaafd3dbf1adf1c78e56f2c Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 24 Aug 2021 19:39:34 +1000 Subject: [PATCH 2/7] Issue #6642 - prevent connection close after websocket upgrade Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpGenerator.java | 12 ++++++++++++ .../jetty/websocket/tests/ConnectionHeaderTest.java | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 00e795a78539..7d21a0bc52c4 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -345,6 +345,18 @@ private Result completing(ByteBuffer chunk, ByteBuffer content) return Result.FLUSH; } + // If this is an upgrade then we don't want to close the connection. + if (_info.isResponse() && ((MetaData.Response)_info).getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) + { + return Result.DONE; + } + else if (_info.isRequest()) + { + HttpField connectionHeader = _info.getFields().getField(HttpHeader.CONNECTION); + if (connectionHeader != null && connectionHeader.contains(HttpHeaderValue.UPGRADE.asString())) + return Result.DONE; + } + _state = State.END; return Boolean.TRUE.equals(_persistent) ? Result.DONE : Result.SHUTDOWN_OUT; } diff --git a/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java index c0391a3fa4be..3bed608a506b 100644 --- a/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java +++ b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConnectionHeaderTest.java @@ -67,7 +67,7 @@ public static void stopContainers() throws Exception } @ParameterizedTest - @ValueSource(strings = {"Upgrade", "keep-alive, Upgrade"}) // TODO: should we test "close, Upgrade", what should we expect? + @ValueSource(strings = {"Upgrade", "keep-alive, Upgrade", "close, Upgrade"}) public void testConnectionKeepAlive(String connectionHeaderValue) throws Exception { URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/echo"); From cb9a8d406064a4b4388d2bb99159ae789d0ff230 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 25 Aug 2021 11:27:56 +1000 Subject: [PATCH 3/7] Issue #6642 - change HttpGenerator state to END before returning Signed-off-by: Lachlan Roberts --- .../src/main/java/org/eclipse/jetty/http/HttpGenerator.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 7d21a0bc52c4..8a35c6c6ed53 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -344,6 +344,7 @@ private Result completing(ByteBuffer chunk, ByteBuffer content) _endOfContent = EndOfContent.UNKNOWN_CONTENT; return Result.FLUSH; } + _state = State.END; // If this is an upgrade then we don't want to close the connection. if (_info.isResponse() && ((MetaData.Response)_info).getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) @@ -356,8 +357,6 @@ else if (_info.isRequest()) if (connectionHeader != null && connectionHeader.contains(HttpHeaderValue.UPGRADE.asString())) return Result.DONE; } - - _state = State.END; return Boolean.TRUE.equals(_persistent) ? Result.DONE : Result.SHUTDOWN_OUT; } From fa316fc20d4ecfc8401e30095d6250251d00f02c Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 26 Aug 2021 13:09:36 +1000 Subject: [PATCH 4/7] Issue #6642 - never shutdown output after generating a request. Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/HttpGenerator.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 8a35c6c6ed53..5cefc3002fe1 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -346,17 +346,14 @@ private Result completing(ByteBuffer chunk, ByteBuffer content) } _state = State.END; - // If this is an upgrade then we don't want to close the connection. + // If this is a request, don't close the connection until the server responds. + if (_info.isRequest()) + return Result.DONE; + + // If successfully upgraded it is responsibility of the next protocol to close the connection. if (_info.isResponse() && ((MetaData.Response)_info).getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) - { return Result.DONE; - } - else if (_info.isRequest()) - { - HttpField connectionHeader = _info.getFields().getField(HttpHeader.CONNECTION); - if (connectionHeader != null && connectionHeader.contains(HttpHeaderValue.UPGRADE.asString())) - return Result.DONE; - } + return Boolean.TRUE.equals(_persistent) ? Result.DONE : Result.SHUTDOWN_OUT; } From 949aa6c3429061f96c4966bb9adbe6bef58af229 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 27 Aug 2021 11:53:54 +1000 Subject: [PATCH 5/7] Issue #6642 - move shutdown logic into HttpChannelOverHTTP and HttpConnection Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/client/http/HttpChannelOverHTTP.java | 2 +- .../main/java/org/eclipse/jetty/http/HttpGenerator.java | 9 --------- .../java/org/eclipse/jetty/server/HttpConnection.java | 5 ++++- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java index 901857f48584..d1750e2bac54 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java @@ -103,7 +103,7 @@ public void exchangeTerminated(HttpExchange exchange, Result result) closeReason = "failure"; else if (receiver.isShutdown()) closeReason = "server close"; - else if (sender.isShutdown()) + else if (sender.isShutdown() && response.getStatus() != HttpStatus.SWITCHING_PROTOCOLS_101) closeReason = "client close"; if (closeReason == null) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 5cefc3002fe1..bce11e13b757 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -345,15 +345,6 @@ private Result completing(ByteBuffer chunk, ByteBuffer content) return Result.FLUSH; } _state = State.END; - - // If this is a request, don't close the connection until the server responds. - if (_info.isRequest()) - return Result.DONE; - - // If successfully upgraded it is responsibility of the next protocol to close the connection. - if (_info.isResponse() && ((MetaData.Response)_info).getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) - return Result.DONE; - return Boolean.TRUE.equals(_persistent) ? Result.DONE : Result.SHUTDOWN_OUT; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 728214a75721..d34b9f7b9241 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpParser; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.io.AbstractConnection; @@ -885,8 +886,10 @@ private void releaseChunk() @Override protected void onCompleteSuccess() { + boolean upgrading = _info.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101; release().succeeded(); - if (_shutdownOut) + // If successfully upgraded it is responsibility of the next protocol to close the connection. + if (_shutdownOut && !upgrading) getEndPoint().shutdownOutput(); } From 2d4693a2887c93eaaf2d4a84521be1e926ac1539 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 27 Aug 2021 14:51:07 +1000 Subject: [PATCH 6/7] Issue #6642 - check for null Metadata in HttpConnection Signed-off-by: Lachlan Roberts --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index d34b9f7b9241..1b027cea629b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -886,7 +886,7 @@ private void releaseChunk() @Override protected void onCompleteSuccess() { - boolean upgrading = _info.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101; + boolean upgrading = _info != null && _info.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101; release().succeeded(); // If successfully upgraded it is responsibility of the next protocol to close the connection. if (_shutdownOut && !upgrading) From edcb215ac7e85aea8d950747a967556db0bc0390 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 31 Aug 2021 10:51:45 +1000 Subject: [PATCH 7/7] Issue #6642 - use request UPGRADE_CONNECTION_ATTRIBUTE instead of 101 response. Signed-off-by: Lachlan Roberts --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 1b027cea629b..3da795a754a0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -28,7 +28,6 @@ import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpParser; -import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.io.AbstractConnection; @@ -886,7 +885,7 @@ private void releaseChunk() @Override protected void onCompleteSuccess() { - boolean upgrading = _info != null && _info.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101; + boolean upgrading = _channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE) != null; release().succeeded(); // If successfully upgraded it is responsibility of the next protocol to close the connection. if (_shutdownOut && !upgrading)