From 2383bf4974ba7d82109cedfc4a8e7693d106abf0 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 1 Jul 2019 16:17:41 +1000 Subject: [PATCH] Issue #3835 - ensure WebSocketSessions are always stopped Signed-off-by: Lachlan Roberts --- .../websocket/jsr356/ClientContainer.java | 5 +++ .../jsr356/server/SessionTrackingTest.java | 5 +++ .../tests/ConcurrentConnectTest.java | 32 +++++++++++++++++++ .../tests/client/BadNetworkTest.java | 5 +++ .../client/ClientOpenSessionTracker.java | 5 +++ .../tests/client/ClientSessionsTest.java | 7 +++- .../client/WebSocketUpgradeRequest.java | 1 - .../websocket/common/SessionTracker.java | 10 +++++- .../websocket/common/WebSocketSession.java | 2 ++ .../common/WebSocketSessionListener.java | 2 ++ .../server/WebSocketServerFactory.java | 1 - 11 files changed, 71 insertions(+), 4 deletions(-) diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java index b1014515a58..ae4ba3c77db 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java @@ -524,6 +524,11 @@ public class ClientContainer extends ContainerLifeCycle implements WebSocketCont this.listener = listener; } + @Override + public void onSessionCreated(WebSocketSession session) + { + } + @Override public void onSessionOpened(WebSocketSession session) { diff --git a/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/SessionTrackingTest.java b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/SessionTrackingTest.java index a26ab461389..41b1cdd2bf8 100644 --- a/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/SessionTrackingTest.java +++ b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/SessionTrackingTest.java @@ -96,6 +96,11 @@ public class SessionTrackingTest CountDownLatch closedLatch = new CountDownLatch(2); wsServerFactory.addSessionListener(new WebSocketSessionListener() { + @Override + public void onSessionCreated(WebSocketSession session) + { + } + @Override public void onSessionOpened(WebSocketSession session) { diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConcurrentConnectTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConcurrentConnectTest.java index b4369a4232b..9a3f2811272 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConcurrentConnectTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/ConcurrentConnectTest.java @@ -21,6 +21,7 @@ package org.eclipse.jetty.websocket.tests; import java.net.URI; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.eclipse.jetty.server.Server; @@ -28,6 +29,8 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.websocket.api.StatusCode; import org.eclipse.jetty.websocket.client.WebSocketClient; +import org.eclipse.jetty.websocket.common.WebSocketSession; +import org.eclipse.jetty.websocket.common.WebSocketSessionListener; import org.eclipse.jetty.websocket.servlet.WebSocketServlet; import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory; import org.junit.jupiter.api.AfterEach; @@ -78,6 +81,8 @@ public class ConcurrentConnectTest public void testConcurrentConnect() throws Exception { List listeners = new ArrayList(); + CloseListener closeListener = new CloseListener(); + client.addSessionListener(closeListener); final int messages = MAX_CONNECTIONS; for (int i = 0; i < messages; i++) @@ -113,6 +118,33 @@ public class ConcurrentConnectTest assertThat(l.closeReason, is("close from client")); //assertNull(l.failure); //TODO: we can get failures after close?? } + + closeListener.closeLatch.await(5, TimeUnit.SECONDS); + for (EventSocket l : listeners) + { + assertTrue(((WebSocketSession)l.session).isStopped()); + } + } + + public static class CloseListener implements WebSocketSessionListener + { + public CountDownLatch closeLatch = new CountDownLatch(MAX_CONNECTIONS); + + @Override + public void onSessionCreated(WebSocketSession session) + { + } + + @Override + public void onSessionOpened(WebSocketSession session) + { + } + + @Override + public void onSessionClosed(WebSocketSession session) + { + closeLatch.countDown(); + } } public static class MyWebSocketServlet extends WebSocketServlet 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 baf3104c903..49c52847bb2 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 @@ -139,6 +139,11 @@ public class BadNetworkTest WebSocketServerFactory wssf = (WebSocketServerFactory)context.getServletContext().getAttribute(WebSocketServletFactory.class.getName()); wssf.addSessionListener(new WebSocketSessionListener() { + @Override + public void onSessionCreated(WebSocketSession session) + { + } + @Override public void onSessionOpened(WebSocketSession session) { diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientOpenSessionTracker.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientOpenSessionTracker.java index 07f03bc248c..efc4c314bee 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientOpenSessionTracker.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientOpenSessionTracker.java @@ -63,6 +63,11 @@ public class ClientOpenSessionTracker implements Connection.Listener, WebSocketS this.closeConnectionLatch.countDown(); } + @Override + public void onSessionCreated(WebSocketSession session) + { + } + @Override public void onSessionOpened(WebSocketSession session) { 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 166a4e816aa..fcdd14347a4 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 @@ -102,6 +102,11 @@ public class ClientSessionsTest client.addSessionListener(new WebSocketSessionListener() { + @Override + public void onSessionCreated(WebSocketSession session) + { + } + @Override public void onSessionOpened(WebSocketSession session) { @@ -134,7 +139,7 @@ public class ClientSessionsTest assertThat("Session.upgradeRequest", sess.getUpgradeRequest(), notNullValue()); assertThat("Session.upgradeResponse", sess.getUpgradeResponse(), notNullValue()); - Collection sessions = client.getBeans(WebSocketSession.class); + Collection sessions = client.getOpenSessions(); assertThat("client.connectionManager.sessions.size", sessions.size(), is(1)); RemoteEndpoint remote = sess.getRemote(); diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketUpgradeRequest.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketUpgradeRequest.java index c5236337dc5..c8b6259e019 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketUpgradeRequest.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketUpgradeRequest.java @@ -634,7 +634,6 @@ public class WebSocketUpgradeRequest extends HttpRequest implements CompleteList session.addManaged(extensionStack); session.setFuture(fut); - wsClient.addManaged(session); if (upgradeListener != null) { diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/SessionTracker.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/SessionTracker.java index f8a5ae08d4a..eb6b7df8af4 100644 --- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/SessionTracker.java +++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/SessionTracker.java @@ -34,16 +34,24 @@ public class SessionTracker extends AbstractLifeCycle implements WebSocketSessio return Collections.unmodifiableSet(sessions); } + @Override + public void onSessionCreated(WebSocketSession session) + { + LifeCycle.start(session); + sessions.add(session); + } + @Override public void onSessionOpened(WebSocketSession session) { - sessions.add(session); + // do nothing } @Override public void onSessionClosed(WebSocketSession session) { sessions.remove(session); + LifeCycle.stop(session); } @Override diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java index 0ead85456ba..700ba25a55c 100644 --- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java +++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java @@ -101,6 +101,8 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Rem addBean(this.connection); addBean(this.websocket); + + notifySessionListeners(containerScope, (listener) -> listener.onSessionCreated(this)); } /** diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSessionListener.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSessionListener.java index 2c1c0814abc..6835d3067f0 100644 --- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSessionListener.java +++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSessionListener.java @@ -20,6 +20,8 @@ package org.eclipse.jetty.websocket.common; public interface WebSocketSessionListener { + void onSessionCreated(WebSocketSession session); + void onSessionOpened(WebSocketSession session); void onSessionClosed(WebSocketSession session); diff --git a/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/WebSocketServerFactory.java b/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/WebSocketServerFactory.java index f98a7edec07..e0def5f4b44 100644 --- a/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/WebSocketServerFactory.java +++ b/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/WebSocketServerFactory.java @@ -663,7 +663,6 @@ public class WebSocketServerFactory extends ContainerLifeCycle implements WebSoc // Start Components session.addManaged(extensionStack); - this.addManaged(session); if (session.isFailed()) {