From 8052c06471693cf0f67187b97e24e35dd74de858 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 17 Oct 2019 19:29:05 +1100 Subject: [PATCH 1/2] Issue #4214 - fix flaky ClientConnectTest and change WS connectTimeout Signed-off-by: Lachlan Roberts --- .../websocket/client/WebSocketClient.java | 3 +- .../tests/client/ClientConnectTest.java | 63 +++++++++++-------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java b/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java index f159c34fbb4f..603fab8bdcc6 100644 --- a/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java +++ b/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java @@ -281,13 +281,14 @@ public long getConnectTimeout() } /** - * Set the timeout for connecting to the remote server. + * Set the timeout for establishing a WebSocket connection with the remote server. * * @param ms the timeout in milliseconds */ public void setConnectTimeout(long ms) { getHttpClient().setConnectTimeout(ms); + getHttpClient().setIdleTimeout(ms); } public CookieStore getCookieStore() diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java index 444eb4485698..540c39a47811 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java @@ -26,6 +26,7 @@ import java.net.URI; import java.time.Duration; import java.util.EnumSet; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -70,6 +71,7 @@ public class ClientConnectTest { private Server server; private WebSocketClient client; + private CountDownLatch serverLatch = new CountDownLatch(1); @SuppressWarnings("unchecked") private E assertExpectedError(ExecutionException e, CloseTrackingEndpoint wsocket, Matcher errorMatcher) @@ -97,7 +99,7 @@ public void startClient() throws Exception { client = new WebSocketClient(); client.setConnectTimeout(TimeUnit.SECONDS.toMillis(3)); - client.setIdleTimeout(Duration.ofSeconds(10)); + client.setIdleTimeout(Duration.ofSeconds(3)); client.start(); } @@ -124,6 +126,19 @@ public void startServer() throws Exception return new EchoSocket(); }); container.addMapping("/get-auth-header", (req, resp) -> new GetAuthHeaderEndpoint()); + + container.addMapping("/noResponse", (req, resp) -> + { + try + { + serverLatch.await(); + } + catch (InterruptedException e) + { + e.printStackTrace(); + } + return null; + }); }); context.addFilter(WebSocketUpgradeFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST)); @@ -367,35 +382,31 @@ public void testConnectionRefused() throws Exception @Test public void testConnectionTimeout_Concurrent() throws Exception { + client.setConnectTimeout(1000); CloseTrackingEndpoint cliSock = new CloseTrackingEndpoint(); - try (ServerSocket serverSocket = new ServerSocket()) - { - InetAddress addr = InetAddress.getByName("localhost"); - InetSocketAddress endpoint = new InetSocketAddress(addr, 0); - serverSocket.bind(endpoint, 1); - int port = serverSocket.getLocalPort(); - URI wsUri = URI.create(String.format("ws://%s:%d/", addr.getHostAddress(), port)); - Future future = client.connect(cliSock, wsUri); + // Connect to endpoint which waits and does not send back a response. + URI wsUri = WSURI.toWebsocket(server.getURI().resolve("/noResponse")); + Future future = client.connect(cliSock, wsUri); - // Accept the connection, but do nothing on it (no response, no upgrade, etc) - serverSocket.accept(); + // The attempt to get upgrade response future should throw error + Exception e = assertThrows(Exception.class, + () -> future.get(5, TimeUnit.SECONDS)); - // The attempt to get upgrade response future should throw error - Exception e = assertThrows(Exception.class, - () -> future.get(5, TimeUnit.SECONDS)); + // Allow server to exit now we have failed. + serverLatch.countDown(); - if (e instanceof ExecutionException) - { - assertExpectedError((ExecutionException)e, cliSock, anyOf( - instanceOf(ConnectException.class), - instanceOf(UpgradeException.class) - )); - } - else - { - assertThat("Should have been a TimeoutException", e, instanceOf(TimeoutException.class)); - } - } + // Unwrap the exception to test if it was what we expected. + assertThat(e, instanceOf(ExecutionException.class)); + + Throwable jettyUpgradeException = e.getCause(); + assertThat(jettyUpgradeException, instanceOf(UpgradeException.class)); + + Throwable coreUpgradeException = jettyUpgradeException.getCause(); + assertThat(coreUpgradeException, instanceOf(org.eclipse.jetty.websocket.core.UpgradeException.class)); + + Throwable timeoutException = coreUpgradeException.getCause(); + assertThat(timeoutException, instanceOf(TimeoutException.class)); + assertThat(timeoutException.getMessage(), containsString("Idle timeout")); } } From 864f07ec7cd3a01338ffa1f464fa0f29e16e8b35 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 17 Oct 2019 20:53:00 +1100 Subject: [PATCH 2/2] Issue #4214 - changes from review Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/websocket/client/WebSocketClient.java | 4 ++-- .../jetty/websocket/tests/client/ClientConnectTest.java | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java b/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java index 603fab8bdcc6..1a90957dddf3 100644 --- a/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java +++ b/jetty-websocket/jetty-websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java @@ -239,6 +239,7 @@ public long getMaxTextMessageSize() public void setIdleTimeout(Duration duration) { configurationCustomizer.setIdleTimeout(duration); + getHttpClient().setIdleTimeout(duration.toMillis()); } @Override @@ -281,14 +282,13 @@ public long getConnectTimeout() } /** - * Set the timeout for establishing a WebSocket connection with the remote server. + * Set the timeout for connecting to the remote server. * * @param ms the timeout in milliseconds */ public void setConnectTimeout(long ms) { getHttpClient().setConnectTimeout(ms); - getHttpClient().setIdleTimeout(ms); } public CookieStore getCookieStore() diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java index 540c39a47811..597af68b6f2d 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java @@ -383,6 +383,7 @@ public void testConnectionRefused() throws Exception public void testConnectionTimeout_Concurrent() throws Exception { client.setConnectTimeout(1000); + client.setIdleTimeout(Duration.ofSeconds(1)); CloseTrackingEndpoint cliSock = new CloseTrackingEndpoint(); // Connect to endpoint which waits and does not send back a response.