From 16fa1133d7933767fbe704826782de1f3eec0323 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 5 Mar 2019 11:30:42 +1100 Subject: [PATCH 1/6] Issue #3406 - enabling all disabled tests in WebSocket Signed-off-by: Lachlan Roberts --- .../tests/server/SessionTrackingTest.java | 29 +++++++++---------- .../tests/client/BadNetworkTest.java | 2 -- .../tests/client/ClientCloseTest.java | 2 -- .../tests/client/ClientSessionsTest.java | 2 -- .../tests/server/ServerCloseTest.java | 2 -- 5 files changed, 14 insertions(+), 23 deletions(-) diff --git a/jetty-websocket/javax-websocket-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/SessionTrackingTest.java b/jetty-websocket/javax-websocket-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/SessionTrackingTest.java index 67109c7a512..71685fa2841 100644 --- a/jetty-websocket/javax-websocket-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/SessionTrackingTest.java +++ b/jetty-websocket/javax-websocket-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/SessionTrackingTest.java @@ -18,20 +18,6 @@ package org.eclipse.jetty.websocket.javax.tests.server; -import org.eclipse.jetty.util.BlockingArrayQueue; -import org.eclipse.jetty.websocket.core.Frame; -import org.eclipse.jetty.websocket.core.OpCode; -import org.eclipse.jetty.websocket.javax.tests.Fuzzer; -import org.eclipse.jetty.websocket.javax.tests.LocalServer; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; - -import javax.websocket.OnMessage; -import javax.websocket.OnOpen; -import javax.websocket.Session; -import javax.websocket.server.ServerEndpoint; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -39,9 +25,22 @@ import java.util.Collection; import java.util.List; import java.util.concurrent.TimeUnit; +import javax.websocket.OnMessage; +import javax.websocket.OnOpen; +import javax.websocket.Session; +import javax.websocket.server.ServerEndpoint; + +import org.eclipse.jetty.util.BlockingArrayQueue; +import org.eclipse.jetty.websocket.core.Frame; +import org.eclipse.jetty.websocket.core.OpCode; +import org.eclipse.jetty.websocket.javax.tests.Fuzzer; +import org.eclipse.jetty.websocket.javax.tests.LocalServer; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + import static org.junit.jupiter.api.Assertions.assertNotNull; -@Disabled("Replaced with ServerCloseTest.testOpenSessionCleanup()") // TODO: Remove once ServerCloseTest is fixed public class SessionTrackingTest { diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/BadNetworkTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/BadNetworkTest.java index 81ef674560d..11ceae1f446 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/BadNetworkTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/BadNetworkTest.java @@ -43,7 +43,6 @@ import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory; import org.eclipse.jetty.websocket.tests.CloseTrackingEndpoint; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.hamcrest.Matchers.containsString; @@ -52,7 +51,6 @@ import static org.hamcrest.Matchers.is; /** * Tests for conditions due to bad networking. */ -@Disabled("Needs triage") public class BadNetworkTest { private Server server; diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java index b2481741586..a48c791cf2d 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java @@ -55,7 +55,6 @@ import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory; import org.eclipse.jetty.websocket.tests.CloseTrackingEndpoint; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.UTF_8; @@ -68,7 +67,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; -@Disabled("Needs triage") public class ClientCloseTest { private Server server; diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java index 8b3b7bf29c0..1d5af427003 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java @@ -46,7 +46,6 @@ import org.eclipse.jetty.websocket.tests.CloseTrackingEndpoint; import org.eclipse.jetty.websocket.tests.EchoCreator; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -55,7 +54,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertTrue; -@Disabled("Needs triage") public class ClientSessionsTest { private Server server; diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java index 33696ccc34b..0b1c6b32482 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java @@ -42,7 +42,6 @@ import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory; import org.eclipse.jetty.websocket.tests.CloseTrackingEndpoint; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static java.util.concurrent.TimeUnit.SECONDS; @@ -55,7 +54,6 @@ import static org.hamcrest.Matchers.is; /** * Tests various close scenarios */ -@Disabled("Needs triage") public class ServerCloseTest { private WebSocketClient client; From 13f8202c069586de7357cd9ee1ad91f4b279b936 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Wed, 6 Mar 2019 11:50:04 +1100 Subject: [PATCH 2/6] Issue #3412 - fix WebSocketClient configuration methods WebSocketClient implements WebSocketPolicy but the methods were never wired through to use the websocket core FrameHandler.Customizer the WSClient now has a ConfigurationCustomizer which it uses to configure the WebSocketCoreClient Signed-off-by: lachan-roberts --- .../websocket/client/WebSocketClient.java | 53 ++++++++----------- 1 file changed, 23 insertions(+), 30 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 00ecfb1fede..4c4a96c5c2f 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 @@ -33,6 +33,7 @@ import java.util.function.Consumer; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.util.DecoratedObjectFactory; import org.eclipse.jetty.util.component.ContainerLifeCycle; import org.eclipse.jetty.util.log.Log; @@ -49,6 +50,7 @@ import org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory; import org.eclipse.jetty.websocket.common.SessionTracker; import org.eclipse.jetty.websocket.common.WebSocketContainer; import org.eclipse.jetty.websocket.common.WebSocketSessionListener; +import org.eclipse.jetty.websocket.core.FrameHandler; import org.eclipse.jetty.websocket.core.WebSocketExtensionRegistry; import org.eclipse.jetty.websocket.core.client.UpgradeListener; import org.eclipse.jetty.websocket.core.client.WebSocketCoreClient; @@ -61,23 +63,17 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli private final JettyWebSocketFrameHandlerFactory frameHandlerFactory; private final List sessionListeners = new CopyOnWriteArrayList<>(); private final SessionTracker sessionTracker = new SessionTracker(); - private ClassLoader contextClassLoader; + private final FrameHandler.ConfigurationCustomizer configurationCustomizer = new FrameHandler.ConfigurationCustomizer(); private DecoratedObjectFactory objectFactory; private WebSocketExtensionRegistry extensionRegistry; - private int inputBufferSize = 4 * 1024; - private int outputBufferSize = 4 * 1024; - private long maxBinaryMessageSize = 64 * 1024; - private long maxTextMessageSize = 64 * 1024; /** * Instantiate a WebSocketClient with defaults */ public WebSocketClient() { - this(new WebSocketCoreClient()); - this.coreClient.getHttpClient().setName("Jetty-WebSocketClient@" + hashCode()); - // We created WebSocketCoreClient, let lifecycle be managed by us - addManaged(coreClient); + this(null); + coreClient.getHttpClient().setName("Jetty-WebSocketClient@" + hashCode()); } /** @@ -87,19 +83,13 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli */ public WebSocketClient(HttpClient httpClient) { - this(new WebSocketCoreClient(httpClient)); - // We created WebSocketCoreClient, let lifecycle be managed by us + coreClient = new WebSocketCoreClient(httpClient, configurationCustomizer); addManaged(coreClient); - } - private WebSocketClient(WebSocketCoreClient coreClient) - { - this.coreClient = coreClient; - this.contextClassLoader = this.getClass().getClassLoader(); - this.objectFactory = new DecoratedObjectFactory(); - this.extensionRegistry = new WebSocketExtensionRegistry(); - this.frameHandlerFactory = new JettyWebSocketFrameHandlerFactory(this); - this.sessionListeners.add(sessionTracker); + objectFactory = new DecoratedObjectFactory(); + extensionRegistry = new WebSocketExtensionRegistry(); + frameHandlerFactory = new JettyWebSocketFrameHandlerFactory(this); + sessionListeners.add(sessionTracker); addBean(sessionTracker); } @@ -134,6 +124,9 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli */ public CompletableFuture connect(Object websocket, URI toUri, UpgradeRequest request, UpgradeListener listener) throws IOException { + for (Connection.Listener listener : getBeans(Connection.Listener.class)) + coreClient.addBean(listener); + JettyClientUpgradeRequest upgradeRequest = new JettyClientUpgradeRequest(this, coreClient, request, toUri, websocket); if (listener != null) upgradeRequest.addListener(listener); @@ -184,61 +177,61 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli @Override public Duration getIdleTimeout() { - return Duration.ofMillis(getHttpClient().getIdleTimeout()); + return configurationCustomizer.getIdleTimeout(); } @Override public int getInputBufferSize() { - return this.inputBufferSize; + return configurationCustomizer.getInputBufferSize(); } @Override public int getOutputBufferSize() { - return this.outputBufferSize; + return configurationCustomizer.getOutputBufferSize(); } @Override public long getMaxBinaryMessageSize() { - return this.maxBinaryMessageSize; + return configurationCustomizer.getMaxBinaryMessageSize(); } @Override public long getMaxTextMessageSize() { - return this.maxTextMessageSize; + return configurationCustomizer.getMaxTextMessageSize(); } @Override public void setIdleTimeout(Duration duration) { - getHttpClient().setIdleTimeout(duration.toMillis()); + configurationCustomizer.setIdleTimeout(duration); } @Override public void setInputBufferSize(int size) { - this.inputBufferSize = size; + configurationCustomizer.setInputBufferSize(size); } @Override public void setOutputBufferSize(int size) { - this.outputBufferSize = size; + configurationCustomizer.setOutputBufferSize(size); } @Override public void setMaxBinaryMessageSize(long size) { - this.maxBinaryMessageSize = size; + configurationCustomizer.setMaxBinaryMessageSize(size); } @Override public void setMaxTextMessageSize(long size) { - this.maxTextMessageSize = size; + configurationCustomizer.setMaxTextMessageSize(size); } public SocketAddress getBindAddress() From 155974bc07e1df6384522984cb7a1f48e1dbe220 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Wed, 6 Mar 2019 13:20:33 +1100 Subject: [PATCH 3/6] Issue #3406 - jetty and javax WebSocket FrameHandler fixes invoke the close handle in onClosed instead of onCloseFrame in JettyWebSocketFrameHandler in both frame handlers replace usage of getClass().getName() with getClass().getSimpleName() to increase the amount of characters from the original exception which can fit in the close frame message Signed-off-by: lachan-roberts --- .../common/JavaxWebSocketFrameHandler.java | 12 ++++--- .../common/JettyWebSocketFrameHandler.java | 35 +++++++++---------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java b/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java index 0a2c8885ad3..c40004e2ae9 100644 --- a/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java +++ b/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java @@ -29,6 +29,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; + import javax.websocket.CloseReason; import javax.websocket.Decoder; import javax.websocket.EndpointConfig; @@ -234,7 +235,7 @@ public class JavaxWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - Exception wse = new WebSocketException(endpointInstance.getClass().getName() + " OPEN method error: " + cause.getMessage(), cause); + Exception wse = new WebSocketException(endpointInstance.getClass().getSimpleName() + " OPEN method error: " + cause.getMessage(), cause); callback.failed(wse); futureSession.completeExceptionally(wse); } @@ -283,12 +284,13 @@ public class JavaxWebSocketFrameHandler implements FrameHandler closeHandle.invoke(closeReason); } callback.succeeded(); - container.notifySessionListeners((listener) -> listener.onJavaxWebSocketSessionClosed(session)); } catch (Throwable cause) { - callback.failed(new WebSocketException(endpointInstance.getClass().getName() + " CLOSE method error: " + cause.getMessage(), cause)); + callback.failed(new WebSocketException(endpointInstance.getClass().getSimpleName() + " CLOSE method error: " + cause.getMessage(), cause)); } + + container.notifySessionListeners((listener) -> listener.onJavaxWebSocketSessionClosed(session)); } @Override @@ -306,7 +308,7 @@ public class JavaxWebSocketFrameHandler implements FrameHandler } catch (Throwable t) { - WebSocketException wsError = new WebSocketException(endpointInstance.getClass().getName() + " ERROR method error: " + cause.getMessage(), t); + WebSocketException wsError = new WebSocketException(endpointInstance.getClass().getSimpleName() + " ERROR method error: " + cause.getMessage(), t); wsError.addSuppressed(cause); callback.failed(wsError); // TODO should futureSession be failed here? @@ -609,7 +611,7 @@ public class JavaxWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - throw new WebSocketException(endpointInstance.getClass().getName() + " PONG method error: " + cause.getMessage(), cause); + throw new WebSocketException(endpointInstance.getClass().getSimpleName() + " PONG method error: " + cause.getMessage(), cause); } } callback.succeeded(); diff --git a/jetty-websocket/jetty-websocket-common/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandler.java b/jetty-websocket/jetty-websocket-common/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandler.java index 40413395f7d..8f402bfd0eb 100644 --- a/jetty-websocket/jetty-websocket-common/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandler.java +++ b/jetty-websocket/jetty-websocket-common/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandler.java @@ -150,7 +150,7 @@ public class JettyWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - callback.failed(new WebSocketException(endpointInstance.getClass().getName() + " OPEN method error: " + cause.getMessage(), cause)); + callback.failed(new WebSocketException(endpointInstance.getClass().getSimpleName() + " OPEN method error: " + cause.getMessage(), cause)); futureSession.completeExceptionally(cause); } } @@ -172,7 +172,7 @@ public class JettyWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - throw new WebSocketException(endpointInstance.getClass().getName() + " FRAME method error: " + cause.getMessage(), cause); + throw new WebSocketException(endpointInstance.getClass().getSimpleName() + " FRAME method error: " + cause.getMessage(), cause); } } @@ -219,7 +219,7 @@ public class JettyWebSocketFrameHandler implements FrameHandler } catch (Throwable t) { - WebSocketException wsError = new WebSocketException(endpointInstance.getClass().getName() + " ERROR method error: " + cause.getMessage(), t); + WebSocketException wsError = new WebSocketException(endpointInstance.getClass().getSimpleName() + " ERROR method error: " + cause.getMessage(), t); wsError.addSuppressed(cause); callback.failed(wsError); } @@ -228,7 +228,18 @@ public class JettyWebSocketFrameHandler implements FrameHandler @Override public void onClosed(CloseStatus closeStatus, Callback callback) { - callback.succeeded(); + try + { + if (closeHandle != null) + closeHandle.invoke(closeStatus.getCode(), closeStatus.getReason()); + + callback.succeeded(); + } + catch (Throwable cause) + { + callback.failed(new WebSocketException(endpointInstance.getClass().getSimpleName() + " CLOSE method error: " + cause.getMessage(), cause)); + } + container.notifySessionListeners((listener) -> listener.onWebSocketSessionClosed(session)); } @@ -262,18 +273,6 @@ public class JettyWebSocketFrameHandler implements FrameHandler private void onCloseFrame(Frame frame, Callback callback) { - if (closeHandle != null) - { - try - { - CloseStatus close = new CloseStatus(frame.getPayload()); - closeHandle.invoke(close.getCode(), close.getReason()); - } - catch (Throwable cause) - { - throw new WebSocketException(endpointInstance.getClass().getName() + " CLOSE method error: " + cause.getMessage(), cause); - } - } callback.succeeded(); } @@ -296,7 +295,7 @@ public class JettyWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - throw new WebSocketException(endpointInstance.getClass().getName() + " PING method error: " + cause.getMessage(), cause); + throw new WebSocketException(endpointInstance.getClass().getSimpleName() + " PING method error: " + cause.getMessage(), cause); } } else @@ -324,7 +323,7 @@ public class JettyWebSocketFrameHandler implements FrameHandler } catch (Throwable cause) { - throw new WebSocketException(endpointInstance.getClass().getName() + " PONG method error: " + cause.getMessage(), cause); + throw new WebSocketException(endpointInstance.getClass().getSimpleName() + " PONG method error: " + cause.getMessage(), cause); } } callback.succeeded(); From 59ec0da76875b725a858b7822d29954694247359 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Thu, 7 Mar 2019 10:52:03 +1100 Subject: [PATCH 4/6] Issue #3406 - fix jetty websocket tests Signed-off-by: lachan-roberts --- .../jetty/websocket/client/WebSocketClient.java | 9 +++++---- .../websocket/tests/client/ClientCloseTest.java | 16 +++++++--------- .../tests/client/ClientSessionsTest.java | 2 ++ .../websocket/tests/server/ServerCloseTest.java | 12 +++++++----- .../core/internal/WebSocketChannelState.java | 2 +- 5 files changed, 22 insertions(+), 19 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 4c4a96c5c2f..999d98bd1b6 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 @@ -118,18 +118,19 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli * @param websocket the websocket object * @param toUri the websocket uri to connect to * @param request the upgrade request information - * @param listener the upgrade listener + * @param upgradeListener the upgrade listener * @return the future for the session, available on success of connect * @throws IOException if unable to connect */ - public CompletableFuture connect(Object websocket, URI toUri, UpgradeRequest request, UpgradeListener listener) throws IOException + public CompletableFuture connect(Object websocket, URI toUri, UpgradeRequest request, UpgradeListener upgradeListener) throws IOException { for (Connection.Listener listener : getBeans(Connection.Listener.class)) coreClient.addBean(listener); JettyClientUpgradeRequest upgradeRequest = new JettyClientUpgradeRequest(this, coreClient, request, toUri, websocket); - if (listener != null) - upgradeRequest.addListener(listener); + if (upgradeListener != null) + upgradeRequest.addListener(upgradeListener); + coreClient.connect(upgradeRequest); return upgradeRequest.getFutureSession(); } diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java index a48c791cf2d..73b06129d0f 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientCloseTest.java @@ -28,7 +28,6 @@ import java.util.Arrays; import java.util.List; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Server; @@ -39,12 +38,12 @@ import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -import org.eclipse.jetty.websocket.api.CloseException; import org.eclipse.jetty.websocket.api.MessageTooLargeException; import org.eclipse.jetty.websocket.api.Session; import org.eclipse.jetty.websocket.api.StatusCode; import org.eclipse.jetty.websocket.api.WebSocketFrameListener; import org.eclipse.jetty.websocket.api.WebSocketListener; +import org.eclipse.jetty.websocket.api.WebSocketTimeoutException; import org.eclipse.jetty.websocket.api.util.WSURI; import org.eclipse.jetty.websocket.client.WebSocketClient; import org.eclipse.jetty.websocket.core.CloseStatus; @@ -250,8 +249,8 @@ public class ClientCloseTest // client reads -1 (EOF) // client triggers close event on client ws-endpoint clientSocket.assertReceivedCloseEvent(clientTimeout * 2, - is(StatusCode.SHUTDOWN), - containsString("timeout")); + is(StatusCode.ABNORMAL), + containsString("Channel Closed")); } clientSessionTracker.assertClosedProperly(client); @@ -283,12 +282,11 @@ public class ClientCloseTest // client close should occur clientSocket.assertReceivedCloseEvent(clientTimeout * 2, is(StatusCode.SHUTDOWN), - containsString("timeout")); + containsString("Timeout")); // client idle timeout triggers close event on client ws-endpoint assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); - assertThat("OnError", clientSocket.error.get(), instanceOf(CloseException.class)); - assertThat("OnError.cause", clientSocket.error.get().getCause(), instanceOf(TimeoutException.class)); + assertThat("OnError", clientSocket.error.get(), instanceOf(WebSocketTimeoutException.class)); } clientSessionTracker.assertClosedProperly(client); @@ -328,7 +326,7 @@ public class ClientCloseTest // clients disconnect for (int i = 0; i < sessionCount; i++) { - clientSockets.get(i).assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Disconnected")); + clientSockets.get(i).assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Channel Closed")); } // ensure all Sessions are gone. connections are gone. etc. (client and server) @@ -368,7 +366,7 @@ public class ClientCloseTest // client triggers close event on client ws-endpoint // assert - close code==1006 (abnormal) - clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Eof")); + clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Channel Closed")); clientSessionTracker.assertClosedProperly(client); } diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java index 1d5af427003..1b74d38df11 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientSessionsTest.java @@ -145,6 +145,8 @@ public class ClientSessionsTest String received = cliSock.messageQueue.poll(5, TimeUnit.SECONDS); assertThat("Message", received, containsString("Hello World!")); + + sess.close(StatusCode.NORMAL, null); } cliSock.assertReceivedCloseEvent(30000, is(StatusCode.NORMAL)); diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java index 0b1c6b32482..722c67c6a08 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java @@ -36,6 +36,7 @@ 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.common.WebSocketSessionImpl; +import org.eclipse.jetty.websocket.core.internal.WebSocketChannel; import org.eclipse.jetty.websocket.server.JettyWebSocketServletContainerInitializer; import org.eclipse.jetty.websocket.servlet.WebSocketServlet; import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory; @@ -148,7 +149,7 @@ public class ServerCloseTest // Verify that server socket got close event AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated(); assertThat("Fast Close Latch", serverEndpoint.closeLatch.await(5, SECONDS), is(true)); - assertThat("Fast Close.statusCode", serverEndpoint.closeStatusCode, is(StatusCode.ABNORMAL)); + assertThat("Fast Close.statusCode", serverEndpoint.closeStatusCode, is(StatusCode.NORMAL)); } finally { @@ -172,7 +173,7 @@ public class ServerCloseTest Future futSession = client.connect(clientEndpoint, wsUri, request); Session session = null; - try(StacklessLogging ignore = new StacklessLogging(FastFailEndpoint.class, WebSocketSessionImpl.class)) + try(StacklessLogging ignore = new StacklessLogging(WebSocketChannel.class)) { session = futSession.get(5, SECONDS); @@ -219,12 +220,13 @@ public class ServerCloseTest clientEndpoint.getEndPoint().close(); // Verify that client got close - clientEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.ABNORMAL), containsString("Disconnected")); + clientEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.ABNORMAL), containsString("Channel Closed")); // Verify that server socket got close event AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated(); - serverEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.ABNORMAL), containsString("Disconnected")); - } finally + serverEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.ABNORMAL), containsString("Channel Closed")); + } + finally { close(session); } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannelState.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannelState.java index 821f5c289fc..76f7bea4400 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannelState.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannelState.java @@ -134,7 +134,7 @@ public class WebSocketChannelState default: if (_closeStatus == null || CloseStatus.isOrdinary(_closeStatus)) - _closeStatus = CloseStatus.NO_CLOSE_STATUS; + _closeStatus = new CloseStatus(CloseStatus.NO_CLOSE, "Channel Closed"); _channelState = State.CLOSED; return true; } From 01bc9d3835b6d32e4c7149c367498a7918b43cd3 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Thu, 7 Mar 2019 14:19:35 +1100 Subject: [PATCH 5/6] Issue #3406 - fix SessionTracker and JettyWebSocketFrameHandlerTest Signed-off-by: lachan-roberts --- .../websocket/javax/common/SessionTracker.java | 3 ++- .../jetty/websocket/common/EventQueue.java | 16 +++++++++++++--- .../common/JettyWebSocketFrameHandlerTest.java | 7 ++++--- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/SessionTracker.java b/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/SessionTracker.java index e2ee48a731d..dd76efa0b95 100644 --- a/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/SessionTracker.java +++ b/jetty-websocket/javax-websocket-common/src/main/java/org/eclipse/jetty/websocket/javax/common/SessionTracker.java @@ -21,6 +21,7 @@ package org.eclipse.jetty.websocket.javax.common; import java.util.Collections; import java.util.Set; import java.util.concurrent.CopyOnWriteArraySet; + import javax.websocket.Session; import org.eclipse.jetty.util.component.AbstractLifeCycle; @@ -44,7 +45,7 @@ public class SessionTracker extends AbstractLifeCycle implements JavaxWebSocketS @Override public void onJavaxWebSocketSessionClosed(JavaxWebSocketSession session) { - sessions.remove(sessions); + sessions.remove(session); } @Override diff --git a/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/EventQueue.java b/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/EventQueue.java index ec2423b3a96..4b0d3d7832f 100644 --- a/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/EventQueue.java +++ b/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/EventQueue.java @@ -19,10 +19,12 @@ package org.eclipse.jetty.websocket.common; import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.concurrent.LinkedBlockingDeque; import static org.eclipse.jetty.toolchain.test.matchers.RegexMatcher.matchesPattern; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.fail; public class EventQueue extends LinkedBlockingDeque { @@ -33,10 +35,18 @@ public class EventQueue extends LinkedBlockingDeque public void assertEvents(String... regexEvents) { - Iterator capturedIterator = iterator(); - for (int i = 0; i < regexEvents.length; i++) + int i = 0; + try { - assertThat("Event [" + i + "]", capturedIterator.next(), matchesPattern(regexEvents[i])); + Iterator capturedIterator = iterator(); + for (i = 0; i < regexEvents.length; i++) + { + assertThat("Event [" + i + "]", capturedIterator.next(), matchesPattern(regexEvents[i])); + } + } + catch (NoSuchElementException e) + { + fail("Event [" + (i) + "] not found: " + regexEvents[i]); } } } diff --git a/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerTest.java b/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerTest.java index eaf3a37fbf5..ca93b5af759 100644 --- a/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerTest.java +++ b/jetty-websocket/jetty-websocket-common/src/test/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerTest.java @@ -121,7 +121,7 @@ public class JettyWebSocketFrameHandlerTest // Trigger Events localEndpoint.onOpen(channel, Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.TEXT).setPayload("Hello?").setFin(true), Callback.NOOP); - localEndpoint.onFrame(CloseStatus.toFrame(StatusCode.NORMAL, "Normal"), Callback.NOOP); + localEndpoint.onClosed(new CloseStatus(StatusCode.NORMAL, "Normal"), Callback.NOOP); // Validate Events socket.events.assertEvents( @@ -216,6 +216,7 @@ public class JettyWebSocketFrameHandlerTest localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload(" the ").setFin(false), Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload("Pig").setFin(true), Callback.NOOP); localEndpoint.onFrame(CloseStatus.toFrame(StatusCode.NORMAL), Callback.NOOP); + localEndpoint.onClosed(CloseStatus.NORMAL_STATUS, Callback.NOOP); // Validate Events socket.events.assertEvents( @@ -245,7 +246,7 @@ public class JettyWebSocketFrameHandlerTest localEndpoint.onFrame(new Frame(OpCode.BINARY).setPayload("Save").setFin(false), Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload(" the ").setFin(false), Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload("Pig").setFin(true), Callback.NOOP); - localEndpoint.onFrame(CloseStatus.toFrame(StatusCode.NORMAL, "Normal"), Callback.NOOP); + localEndpoint.onClosed(new CloseStatus(StatusCode.NORMAL, "Normal"), Callback.NOOP); // Validate Events socket.events.assertEvents( @@ -323,7 +324,7 @@ public class JettyWebSocketFrameHandlerTest localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload(" the ").setFin(false), Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.PONG).setPayload("You there?"), Callback.NOOP); localEndpoint.onFrame(new Frame(OpCode.CONTINUATION).setPayload("Pig").setFin(true), Callback.NOOP); - localEndpoint.onFrame(CloseStatus.toFrame(StatusCode.NORMAL, "Normal"), Callback.NOOP); + localEndpoint.onClosed(new CloseStatus(StatusCode.NORMAL, "Normal"), Callback.NOOP); // Validate Events socket.events.assertEvents( From ddd60f1992fe451ac017713f7efb8cde48cf2263 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Fri, 8 Mar 2019 14:50:49 +1100 Subject: [PATCH 6/6] Issue #3406 - changes from review Signed-off-by: lachan-roberts --- .../org/eclipse/jetty/websocket/client/WebSocketClient.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 999d98bd1b6..187f7437fa4 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 @@ -73,7 +73,6 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli public WebSocketClient() { this(null); - coreClient.getHttpClient().setName("Jetty-WebSocketClient@" + hashCode()); } /** @@ -86,6 +85,9 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketPoli coreClient = new WebSocketCoreClient(httpClient, configurationCustomizer); addManaged(coreClient); + if (httpClient == null) + coreClient.getHttpClient().setName("Jetty-WebSocketClient@" + hashCode()); + objectFactory = new DecoratedObjectFactory(); extensionRegistry = new WebSocketExtensionRegistry(); frameHandlerFactory = new JettyWebSocketFrameHandlerFactory(this);