From 6a8a6efd1cfc591a224cc053c547695c5fb27172 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Sat, 4 Jul 2020 11:37:45 +1000 Subject: [PATCH 1/4] Issue #5018 - add request timeout onto ClientUpgradeRequest Signed-off-by: Lachlan Roberts --- .../client/ClientUpgradeRequest.java | 23 +++++++++++++++++++ .../websocket/client/WebSocketClient.java | 3 ++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java index 76724b08673..854f1022701 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java @@ -30,6 +30,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.http.HttpField; @@ -68,6 +69,7 @@ public class ClientUpgradeRequest extends UpgradeRequestAdapter private final String key; private Object localEndpoint; + private long timeout; public ClientUpgradeRequest() { @@ -179,6 +181,27 @@ public class ClientUpgradeRequest extends UpgradeRequestAdapter } } + /** + * @param timeout the total timeout for the request/response conversation of the WebSocket handshake; + * use zero or a negative value to disable the timeout + * @param unit the timeout unit + * @return this request object + */ + public ClientUpgradeRequest timeout(long timeout, TimeUnit unit) + { + this.timeout = unit.toMillis(timeout); + return this; + } + + /** + * @return the total timeout for this request, in milliseconds; + * zero or negative if the timeout is disabled + */ + public long getTimeout() + { + return timeout; + } + public void setLocalEndpoint(Object websocket) { this.localEndpoint = websocket; diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java index 514e51a8b59..c0ed249a719 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java @@ -30,6 +30,7 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.Executor; import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.function.Supplier; import org.eclipse.jetty.client.HttpClient; @@ -374,7 +375,7 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketCont init(); WebSocketUpgradeRequest wsReq = new WebSocketUpgradeRequest(this, httpClient, request); - + wsReq.timeout(request.getTimeout() , TimeUnit.MILLISECONDS); wsReq.setUpgradeListener(upgradeListener); return wsReq.sendAsync(); } From fbe3803cd1da19f2de0be0bf58a08fe3ebe1973b Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 6 Jul 2020 12:19:38 +1000 Subject: [PATCH 2/4] fix checkstyle issue Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/websocket/client/WebSocketClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java index c0ed249a719..bbd6cd84bb6 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java @@ -375,7 +375,7 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketCont init(); WebSocketUpgradeRequest wsReq = new WebSocketUpgradeRequest(this, httpClient, request); - wsReq.timeout(request.getTimeout() , TimeUnit.MILLISECONDS); + wsReq.timeout(request.getTimeout(), TimeUnit.MILLISECONDS); wsReq.setUpgradeListener(upgradeListener); return wsReq.sendAsync(); } From 74916c2fc946d6b83c8ce6f1a6ed591218ac7fa2 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 7 Jul 2020 16:38:51 +1000 Subject: [PATCH 3/4] Issue #5018 - add test case for ClientUpgradeRequest timeout Signed-off-by: Lachlan Roberts --- .../tests/client/ClientTimeoutTest.java | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java new file mode 100644 index 00000000000..d18e59ad8ea --- /dev/null +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java @@ -0,0 +1,123 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.websocket.tests.client; + +import java.util.EnumSet; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import javax.servlet.DispatcherType; + +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.api.UpgradeException; +import org.eclipse.jetty.websocket.api.util.WSURI; +import org.eclipse.jetty.websocket.client.ClientUpgradeRequest; +import org.eclipse.jetty.websocket.client.WebSocketClient; +import org.eclipse.jetty.websocket.server.NativeWebSocketServletContainerInitializer; +import org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter; +import org.eclipse.jetty.websocket.tests.EventSocket; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class ClientTimeoutTest +{ + private Server server; + private WebSocketClient client; + private final CountDownLatch createEndpoint = new CountDownLatch(1); + + @BeforeEach + public void start() throws Exception + { + server = new Server(); + ServerConnector connector = new ServerConnector(server); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + server.setHandler(contextHandler); + + NativeWebSocketServletContainerInitializer.configure(contextHandler, (context, container) -> + { + container.addMapping("/", (req, res) -> + { + try + { + createEndpoint.await(5, TimeUnit.SECONDS); + return new EventSocket.EchoSocket(); + } + catch (InterruptedException e) + { + throw new IllegalStateException(e); + } + }); + }); + contextHandler.addFilter(WebSocketUpgradeFilter.class, "/", EnumSet.of(DispatcherType.REQUEST)); + server.start(); + + client = new WebSocketClient(); + client.start(); + } + + @AfterEach + public void stop() throws Exception + { + createEndpoint.countDown(); + client.stop(); + server.stop(); + } + + @Test + public void testWebSocketClientTimeout() throws Exception + { + EventSocket clientSocket = new EventSocket(); + long timeout = 1000; + client.setMaxIdleTimeout(timeout); + Future connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI())); + + ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout + 200, TimeUnit.MILLISECONDS)); + assertThat(executionException.getCause(), instanceOf(UpgradeException.class)); + UpgradeException upgradeException = (UpgradeException)executionException.getCause(); + assertThat(upgradeException.getCause(), instanceOf(TimeoutException.class)); + } + + @Test + public void testClientUpgradeRequestTimeout() throws Exception + { + EventSocket clientSocket = new EventSocket(); + long timeout = 1000; + ClientUpgradeRequest upgradeRequest = new ClientUpgradeRequest(); + upgradeRequest.timeout(timeout, TimeUnit.MILLISECONDS); + Future connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI()), upgradeRequest); + + ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout + 200, TimeUnit.MILLISECONDS)); + assertThat(executionException.getCause(), instanceOf(UpgradeException.class)); + UpgradeException upgradeException = (UpgradeException)executionException.getCause(); + assertThat(upgradeException.getCause(), instanceOf(TimeoutException.class)); + } +} From 702a48c2d128187cf106ad49ba04ae1f580f5c80 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 7 Jul 2020 22:05:01 +1000 Subject: [PATCH 4/4] Issue #5018 - rename ClientUpgradeRequest timeout to setTimeout and increase timeouts used for testing Signed-off-by: Lachlan Roberts --- .../jetty/websocket/tests/client/ClientTimeoutTest.java | 6 +++--- .../jetty/websocket/client/ClientUpgradeRequest.java | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java index d18e59ad8ea..8cc2a7e70fa 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientTimeoutTest.java @@ -100,7 +100,7 @@ public class ClientTimeoutTest client.setMaxIdleTimeout(timeout); Future connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI())); - ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout + 200, TimeUnit.MILLISECONDS)); + ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout * 2, TimeUnit.MILLISECONDS)); assertThat(executionException.getCause(), instanceOf(UpgradeException.class)); UpgradeException upgradeException = (UpgradeException)executionException.getCause(); assertThat(upgradeException.getCause(), instanceOf(TimeoutException.class)); @@ -112,10 +112,10 @@ public class ClientTimeoutTest EventSocket clientSocket = new EventSocket(); long timeout = 1000; ClientUpgradeRequest upgradeRequest = new ClientUpgradeRequest(); - upgradeRequest.timeout(timeout, TimeUnit.MILLISECONDS); + upgradeRequest.setTimeout(timeout, TimeUnit.MILLISECONDS); Future connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI()), upgradeRequest); - ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout + 200, TimeUnit.MILLISECONDS)); + ExecutionException executionException = assertThrows(ExecutionException.class, () -> connect.get(timeout * 2, TimeUnit.MILLISECONDS)); assertThat(executionException.getCause(), instanceOf(UpgradeException.class)); UpgradeException upgradeException = (UpgradeException)executionException.getCause(); assertThat(upgradeException.getCause(), instanceOf(TimeoutException.class)); diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java index 854f1022701..0fcbaa43670 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java @@ -185,12 +185,10 @@ public class ClientUpgradeRequest extends UpgradeRequestAdapter * @param timeout the total timeout for the request/response conversation of the WebSocket handshake; * use zero or a negative value to disable the timeout * @param unit the timeout unit - * @return this request object */ - public ClientUpgradeRequest timeout(long timeout, TimeUnit unit) + public void setTimeout(long timeout, TimeUnit unit) { this.timeout = unit.toMillis(timeout); - return this; } /**