From 50c193c23b2cca6105ec919981cb459546e40e39 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Mon, 1 Apr 2019 11:19:23 +1100 Subject: [PATCH 1/4] Issue #3494 - fix ClientCloseTest.testWriteException() Nulling out values in WebSocketAdapter causes race conditions when trying to access session and endpoint externally Race condition in WebSocketChannel.Flusher.onCompleteFailure(), processConnectionError should be called first to ensure that the correct close reason is processed, super.onCompleteFailure() was closing the connection causing a read failure. race condition between the server detecting a read failure and sending a response and the client detecting the write failure, now blocking on the server so it is not reading and will not detect the failure Signed-off-by: lachan-roberts --- .../jetty/websocket/api/WebSocketAdapter.java | 3 +- .../tests/client/ClientCloseTest.java | 61 ++++++++++++------- .../core/internal/WebSocketChannel.java | 9 +++ .../core/internal/WebSocketConnection.java | 2 +- 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/jetty-websocket/jetty-websocket-api/src/main/java/org/eclipse/jetty/websocket/api/WebSocketAdapter.java b/jetty-websocket/jetty-websocket-api/src/main/java/org/eclipse/jetty/websocket/api/WebSocketAdapter.java index 3406d895053..ef466ed1c93 100644 --- a/jetty-websocket/jetty-websocket-api/src/main/java/org/eclipse/jetty/websocket/api/WebSocketAdapter.java +++ b/jetty-websocket/jetty-websocket-api/src/main/java/org/eclipse/jetty/websocket/api/WebSocketAdapter.java @@ -59,8 +59,7 @@ public class WebSocketAdapter implements WebSocketListener @Override public void onWebSocketClose(int statusCode, String reason) { - this.session = null; - this.remote = null; + /* do nothing */ } @Override 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 b0e43f7d5c1..a2707b5ca69 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 @@ -21,15 +21,16 @@ package org.eclipse.jetty.websocket.tests.client; import java.io.IOException; import java.net.URI; -import java.nio.channels.ClosedChannelException; import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.DefaultHandler; @@ -66,10 +67,12 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ClientCloseTest { private Server server; + private ServerEndpoint serverEndpoint = new ServerEndpoint(); private WebSocketClient client; private Session confirmConnection(CloseTrackingEndpoint clientSocket, Future clientFuture) throws Exception @@ -125,9 +128,9 @@ public class ClientCloseTest @Override public void configure(JettyWebSocketServletFactory factory) { - factory.setIdleTimeout(Duration.ofSeconds(10)); + factory.setIdleTimeout(Duration.ofSeconds(0)); factory.setMaxTextMessageSize(1024 * 1024 * 2); - factory.register(ServerEndpoint.class); + factory.setCreator((req,resp)->serverEndpoint); } }); context.addServlet(holder, "/ws"); @@ -339,8 +342,7 @@ public class ClientCloseTest public void testWriteException() throws Exception { // Set client timeout - final int timeout = 2000; - client.setIdleTimeout(Duration.ofMillis(timeout)); + client.setIdleTimeout(Duration.ZERO); ClientOpenSessionTracker clientSessionTracker = new ClientOpenSessionTracker(1); clientSessionTracker.addTo(client); @@ -353,29 +355,40 @@ public class ClientCloseTest // client confirms connection via echo confirmConnection(clientSocket, clientConnectFuture); - // setup client endpoint for write failure (test only) - EndPoint endp = clientSocket.getEndPoint(); - endp.shutdownOutput(); + try + { + // Block on the server so that the server does not detect a read failure + clientSocket.getSession().getRemote().sendString("block"); - // client enqueue close frame - // should result in a client write failure - final String origCloseReason = "Normal Close from Client"; - clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); + // setup client endpoint for write failure (test only) + EndPoint endp = clientSocket.getEndPoint(); + endp.shutdownOutput(); - assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); - assertThat("OnError", clientSocket.error.get(), instanceOf(ClosedChannelException.class)); + // client enqueue close frame + // should result in a client write failure + final String origCloseReason = "Normal Close from Client"; + clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); - // client triggers close event on client ws-endpoint - // assert - close code==1006 (abnormal) - clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Channel Closed")); + assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); + assertThat("OnError", clientSocket.error.get(), instanceOf(EofException.class)); - clientSessionTracker.assertClosedProperly(client); + // client triggers close event on client ws-endpoint + // assert - close code==1006 (abnormal) + clientSocket.assertReceivedCloseEvent(2000, is(StatusCode.ABNORMAL), null); + + clientSessionTracker.assertClosedProperly(client); + } + finally + { + serverEndpoint.block.countDown(); + } } public static class ServerEndpoint implements WebSocketFrameListener, WebSocketListener { private static final Logger LOG = Log.getLogger(ServerEndpoint.class); private Session session; + CountDownLatch block = new CountDownLatch(1); @Override public void onWebSocketBinary(byte[] payload, int offset, int len) @@ -395,12 +408,20 @@ public class ClientCloseTest String bigmsg = new String(buf, UTF_8); session.getRemote().sendString(bigmsg); } + else if (message.equals("block")) + { + assertTrue(block.await(5, TimeUnit.MINUTES)); + } else { // simple echo session.getRemote().sendString(message); } } + catch (InterruptedException e) + { + throw new IllegalStateException(e); + } catch (IOException ignore) { LOG.debug(ignore); @@ -422,9 +443,7 @@ public class ClientCloseTest public void onWebSocketError(Throwable cause) { if (LOG.isDebugEnabled()) - { - LOG.debug(cause); - } + LOG.debug("onWebSocketError(): ", cause); } @Override diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java index 066fc6ce36f..4a34ebe776d 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java @@ -32,6 +32,7 @@ import java.util.concurrent.Executor; import java.util.concurrent.TimeoutException; import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IteratingCallback; import org.eclipse.jetty.util.Utf8Appendable; @@ -306,12 +307,18 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio public void onEof() { + if (LOG.isDebugEnabled()) + LOG.debug("onEof() {}", this); + if (channelState.onEof()) closeConnection(new ClosedChannelException(), channelState.getCloseStatus(), Callback.NOOP); } public void closeConnection(Throwable cause, CloseStatus closeStatus, Callback callback) { + if (LOG.isDebugEnabled()) + LOG.debug("closeConnection() {} {} {}", closeStatus, this, cause); + connection.cancelDemand(); if (connection.getEndPoint().isOpen()) connection.close(); @@ -371,6 +378,8 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio code = CloseStatus.BAD_PAYLOAD; else if (cause instanceof WebSocketTimeoutException || cause instanceof TimeoutException || cause instanceof SocketTimeoutException) code = CloseStatus.SHUTDOWN; + else if (cause instanceof EofException) + code = CloseStatus.NO_CLOSE; else if (behavior == Behavior.CLIENT) code = CloseStatus.POLICY_VIOLATION; else diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketConnection.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketConnection.java index ad48861a46b..8765d371600 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketConnection.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketConnection.java @@ -599,8 +599,8 @@ public class WebSocketConnection extends AbstractConnection implements Connectio @Override public void onCompleteFailure(Throwable x) { - super.onCompleteFailure(x); channel.processConnectionError(x, NOOP); + super.onCompleteFailure(x); } } } From 607a870d8e171b16c30bf82fb7c97043b829cf39 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 3 Apr 2019 10:34:53 +1100 Subject: [PATCH 2/4] Issue #3494 - fixes to testStopLifecycle Signed-off-by: Lachlan Roberts --- .../tests/client/ClientCloseTest.java | 72 +++++++++++-------- 1 file changed, 42 insertions(+), 30 deletions(-) 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 a2707b5ca69..3b1ee67eb72 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 @@ -37,6 +37,7 @@ import org.eclipse.jetty.server.handler.DefaultHandler; import org.eclipse.jetty.server.handler.HandlerList; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.websocket.api.Frame; @@ -72,8 +73,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class ClientCloseTest { private Server server; - private ServerEndpoint serverEndpoint = new ServerEndpoint(); private WebSocketClient client; + private BlockingArrayQueue serverEndpoints = new BlockingArrayQueue<>(); private Session confirmConnection(CloseTrackingEndpoint clientSocket, Future clientFuture) throws Exception { @@ -128,9 +129,14 @@ public class ClientCloseTest @Override public void configure(JettyWebSocketServletFactory factory) { - factory.setIdleTimeout(Duration.ofSeconds(0)); + factory.setIdleTimeout(Duration.ofSeconds(10)); factory.setMaxTextMessageSize(1024 * 1024 * 2); - factory.setCreator((req,resp)->serverEndpoint); + factory.setCreator((req,resp)-> + { + ServerEndpoint endpoint = new ServerEndpoint(); + serverEndpoints.offer(endpoint); + return endpoint; + }); } }); context.addServlet(holder, "/ws"); @@ -300,7 +306,7 @@ public class ClientCloseTest public void testStopLifecycle() throws Exception { // Set client timeout - final int timeout = 1000; + final int timeout = 3000; client.setIdleTimeout(Duration.ofMillis(timeout)); int sessionCount = 3; @@ -322,6 +328,12 @@ public class ClientCloseTest confirmConnection(clientSocket, clientConnectFuture); } + assertThat(serverEndpoints.size(), is(sessionCount)); + + // block all the server threads + for (int i = 0; i < sessionCount; i++) + clientSockets.get(i).getSession().getRemote().sendString("block"); + assertTimeoutPreemptively(ofSeconds(5), () -> { // client lifecycle stop (the meat of this test) client.stop(); @@ -329,20 +341,22 @@ public class ClientCloseTest // clients disconnect for (int i = 0; i < sessionCount; i++) - { - clientSockets.get(i).assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), containsString("Channel Closed")); - } + clientSockets.get(i).assertReceivedCloseEvent(2000, is(StatusCode.ABNORMAL), containsString("Channel Closed")); // ensure all Sessions are gone. connections are gone. etc. (client and server) // ensure ConnectionListener onClose is called 3 times clientSessionTracker.assertClosedProperly(client); + + for (int i = 0; i < sessionCount; i++) + serverEndpoints.get(i).block.countDown(); } @Test public void testWriteException() throws Exception { // Set client timeout - client.setIdleTimeout(Duration.ZERO); + final int timeout = 2000; + client.setIdleTimeout(Duration.ofMillis(timeout)); ClientOpenSessionTracker clientSessionTracker = new ClientOpenSessionTracker(1); clientSessionTracker.addTo(client); @@ -355,33 +369,28 @@ public class ClientCloseTest // client confirms connection via echo confirmConnection(clientSocket, clientConnectFuture); - try - { - // Block on the server so that the server does not detect a read failure - clientSocket.getSession().getRemote().sendString("block"); + // Block on the server so that the server does not detect a read failure + clientSocket.getSession().getRemote().sendString("block"); - // setup client endpoint for write failure (test only) - EndPoint endp = clientSocket.getEndPoint(); - endp.shutdownOutput(); + // setup client endpoint for write failure (test only) + EndPoint endp = clientSocket.getEndPoint(); + endp.shutdownOutput(); - // client enqueue close frame - // should result in a client write failure - final String origCloseReason = "Normal Close from Client"; - clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); + // client enqueue close frame + // should result in a client write failure + final String origCloseReason = "Normal Close from Client"; + clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); - assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); - assertThat("OnError", clientSocket.error.get(), instanceOf(EofException.class)); + assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); + assertThat("OnError", clientSocket.error.get(), instanceOf(EofException.class)); - // client triggers close event on client ws-endpoint - // assert - close code==1006 (abnormal) - clientSocket.assertReceivedCloseEvent(2000, is(StatusCode.ABNORMAL), null); + // client triggers close event on client ws-endpoint + // assert - close code==1006 (abnormal) + clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), null); + clientSessionTracker.assertClosedProperly(client); - clientSessionTracker.assertClosedProperly(client); - } - finally - { - serverEndpoint.block.countDown(); - } + assertThat(serverEndpoints.size(), is(1)); + serverEndpoints.get(0).block.countDown(); } public static class ServerEndpoint implements WebSocketFrameListener, WebSocketListener @@ -410,7 +419,9 @@ public class ClientCloseTest } else if (message.equals("block")) { + LOG.debug("blocking"); assertTrue(block.await(5, TimeUnit.MINUTES)); + LOG.debug("unblocked"); } else { @@ -420,6 +431,7 @@ public class ClientCloseTest } catch (InterruptedException e) { + LOG.debug("unblocked"); throw new IllegalStateException(e); } catch (IOException ignore) From 7df8140137cd3d519c0b5ef136e70706789b318c Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 3 Apr 2019 14:50:37 +1100 Subject: [PATCH 3/4] Issue #3494 - failures in testRemoteDisconnect because of timeout Signed-off-by: Lachlan Roberts --- .../tests/client/ClientCloseTest.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) 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 3b1ee67eb72..395afc771eb 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 @@ -237,7 +237,7 @@ public class ClientCloseTest public void testRemoteDisconnect() throws Exception { // Set client timeout - final int clientTimeout = 1000; + final int clientTimeout = 3000; client.setIdleTimeout(Duration.ofMillis(clientTimeout)); ClientOpenSessionTracker clientSessionTracker = new ClientOpenSessionTracker(1); @@ -248,20 +248,18 @@ public class ClientCloseTest CloseTrackingEndpoint clientSocket = new CloseTrackingEndpoint(); Future clientConnectFuture = client.connect(clientSocket, wsUri); - try (Session ignored = confirmConnection(clientSocket, clientConnectFuture)) - { - // client confirms connection via echo + // client confirms connection via echo + confirmConnection(clientSocket, clientConnectFuture); - // client sends close frame (triggering server connection abort) - final String origCloseReason = "abort"; - clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); + // client sends close frame (triggering server connection abort) + final String origCloseReason = "abort"; + clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); - // client reads -1 (EOF) - // client triggers close event on client ws-endpoint - clientSocket.assertReceivedCloseEvent(clientTimeout * 2, - is(StatusCode.ABNORMAL), - containsString("Channel Closed")); - } + // client reads -1 (EOF) + // client triggers close event on client ws-endpoint + clientSocket.assertReceivedCloseEvent(2000, + is(StatusCode.ABNORMAL), + containsString("Channel Closed")); clientSessionTracker.assertClosedProperly(client); } @@ -286,7 +284,7 @@ public class ClientCloseTest // client confirms connection via echo // client sends close frame - final String origCloseReason = "sleep|5000"; + final String origCloseReason = "sleep|2500"; clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); // client close should occur From 6cd5fb42e9d133ac535c0f809b81d527356f2516 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 5 Apr 2019 12:18:09 +1100 Subject: [PATCH 4/4] Issue #3494 - adjustments to WebSocketChannel processConnectionError WebSocketChannel.processConnectionError now defaults to NO_CLOSE status if no protocol reasons can be found added some debug logging improvements to tests Signed-off-by: Lachlan Roberts --- .../websocket/tests/WebSocketStatsTest.java | 2 +- .../tests/client/ClientCloseTest.java | 79 +++++++++++-------- .../core/internal/WebSocketChannel.java | 72 +++++++++-------- .../websocket/core/WebSocketCloseTest.java | 29 ++++--- 4 files changed, 102 insertions(+), 80 deletions(-) diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketStatsTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketStatsTest.java index 795120cbd29..226ff8f2e01 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketStatsTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketStatsTest.java @@ -179,7 +179,7 @@ public class WebSocketStatsTest ClientSocket socket = new ClientSocket(); CompletableFuture connect = client.connect(socket, uri); - final long numMessages = 10000; + final long numMessages = 1000; final String msgText = "hello world"; long upgradeSentBytes; 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 395afc771eb..c7a231017ab 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 @@ -328,25 +328,33 @@ public class ClientCloseTest assertThat(serverEndpoints.size(), is(sessionCount)); - // block all the server threads - for (int i = 0; i < sessionCount; i++) - clientSockets.get(i).getSession().getRemote().sendString("block"); + try + { + // block all the server threads + for (int i = 0; i < sessionCount; i++) + clientSockets.get(i).getSession().getRemote().sendString("block"); - assertTimeoutPreemptively(ofSeconds(5), () -> { - // client lifecycle stop (the meat of this test) - client.stop(); - }); + assertTimeoutPreemptively(ofSeconds(5), () -> + { + // client lifecycle stop (the meat of this test) + client.stop(); + }); - // clients disconnect - for (int i = 0; i < sessionCount; i++) - clientSockets.get(i).assertReceivedCloseEvent(2000, is(StatusCode.ABNORMAL), containsString("Channel Closed")); + // clients disconnect + for (int i = 0; i < sessionCount; i++) + clientSockets.get(i).assertReceivedCloseEvent(2000, is(StatusCode.ABNORMAL), containsString("Channel Closed")); - // ensure all Sessions are gone. connections are gone. etc. (client and server) - // ensure ConnectionListener onClose is called 3 times - clientSessionTracker.assertClosedProperly(client); + // ensure all Sessions are gone. connections are gone. etc. (client and server) + // ensure ConnectionListener onClose is called 3 times + clientSessionTracker.assertClosedProperly(client); - for (int i = 0; i < sessionCount; i++) - serverEndpoints.get(i).block.countDown(); + assertThat(serverEndpoints.size(), is(sessionCount)); + } + finally + { + for (int i = 0; i < sessionCount; i++) + serverEndpoints.get(i).block.countDown(); + } } @Test @@ -367,28 +375,35 @@ public class ClientCloseTest // client confirms connection via echo confirmConnection(clientSocket, clientConnectFuture); - // Block on the server so that the server does not detect a read failure - clientSocket.getSession().getRemote().sendString("block"); + try + { + // Block on the server so that the server does not detect a read failure + clientSocket.getSession().getRemote().sendString("block"); - // setup client endpoint for write failure (test only) - EndPoint endp = clientSocket.getEndPoint(); - endp.shutdownOutput(); + // setup client endpoint for write failure (test only) + EndPoint endp = clientSocket.getEndPoint(); + endp.shutdownOutput(); - // client enqueue close frame - // should result in a client write failure - final String origCloseReason = "Normal Close from Client"; - clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); + // client enqueue close frame + // should result in a client write failure + final String origCloseReason = "Normal Close from Client"; + clientSocket.getSession().close(StatusCode.NORMAL, origCloseReason); - assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); - assertThat("OnError", clientSocket.error.get(), instanceOf(EofException.class)); + assertThat("OnError Latch", clientSocket.errorLatch.await(2, SECONDS), is(true)); + assertThat("OnError", clientSocket.error.get(), instanceOf(EofException.class)); - // client triggers close event on client ws-endpoint - // assert - close code==1006 (abnormal) - clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), null); - clientSessionTracker.assertClosedProperly(client); + // client triggers close event on client ws-endpoint + // assert - close code==1006 (abnormal) + clientSocket.assertReceivedCloseEvent(timeout, is(StatusCode.ABNORMAL), null); + clientSessionTracker.assertClosedProperly(client); - assertThat(serverEndpoints.size(), is(1)); - serverEndpoints.get(0).block.countDown(); + assertThat(serverEndpoints.size(), is(1)); + } + finally + { + for (ServerEndpoint endpoint : serverEndpoints) + endpoint.block.countDown(); + } } public static class ServerEndpoint implements WebSocketFrameListener, WebSocketListener diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java index 4a34ebe776d..335da69d1cb 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketChannel.java @@ -32,7 +32,6 @@ import java.util.concurrent.Executor; import java.util.concurrent.TimeoutException; import org.eclipse.jetty.io.ByteBufferPool; -import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IteratingCallback; import org.eclipse.jetty.util.Utf8Appendable; @@ -363,29 +362,6 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio callback.failed(e); } } - - - } - - AbnormalCloseStatus abnormalCloseStatusFor(Throwable cause) - { - int code; - if (cause instanceof ProtocolException) - code = CloseStatus.PROTOCOL; - else if (cause instanceof CloseException) - code = ((CloseException)cause).getStatusCode(); - else if (cause instanceof Utf8Appendable.NotUtf8Exception) - code = CloseStatus.BAD_PAYLOAD; - else if (cause instanceof WebSocketTimeoutException || cause instanceof TimeoutException || cause instanceof SocketTimeoutException) - code = CloseStatus.SHUTDOWN; - else if (cause instanceof EofException) - code = CloseStatus.NO_CLOSE; - else if (behavior == Behavior.CLIENT) - code = CloseStatus.POLICY_VIOLATION; - else - code = CloseStatus.SERVER_ERROR; - - return new AbnormalCloseStatus(code, cause); } /** @@ -401,14 +377,24 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio if (LOG.isDebugEnabled()) LOG.debug("processConnectionError {} {}", this, cause); - CloseStatus closeStatus = abnormalCloseStatusFor(cause); - - if (closeStatus.getCode() == CloseStatus.PROTOCOL) - close(closeStatus, callback); - else if (channelState.onClosed(closeStatus)) - closeConnection(cause, closeStatus, callback); + int code; + if (cause instanceof CloseException) + code = ((CloseException)cause).getStatusCode(); + else if (cause instanceof Utf8Appendable.NotUtf8Exception) + code = CloseStatus.BAD_PAYLOAD; + else if (cause instanceof WebSocketTimeoutException || cause instanceof TimeoutException || cause instanceof SocketTimeoutException) + code = CloseStatus.SHUTDOWN; else - callback.failed(cause); + code = CloseStatus.NO_CLOSE; + + AbnormalCloseStatus closeStatus = new AbnormalCloseStatus(code, cause); + if (CloseStatus.isTransmittableStatusCode(code)) + close(closeStatus, callback); + else + { + if (channelState.onClosed(closeStatus)) + closeConnection(cause, closeStatus, callback); + } } /** @@ -423,7 +409,19 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio if (LOG.isDebugEnabled()) LOG.debug("processHandlerError {} {}", this, cause); - close(abnormalCloseStatusFor(cause), callback); + int code; + if (cause instanceof CloseException) + code = ((CloseException)cause).getStatusCode(); + else if (cause instanceof Utf8Appendable.NotUtf8Exception) + code = CloseStatus.BAD_PAYLOAD; + else if (cause instanceof WebSocketTimeoutException || cause instanceof TimeoutException || cause instanceof SocketTimeoutException) + code = CloseStatus.SHUTDOWN; + else if (behavior == Behavior.CLIENT) + code = CloseStatus.POLICY_VIOLATION; + else + code = CloseStatus.SERVER_ERROR; + + close(new AbnormalCloseStatus(code, cause), callback); } /** @@ -515,6 +513,9 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio } catch (Throwable t) { + if (LOG.isDebugEnabled()) + LOG.warn("Invalid outgoing frame: {}", frame); + callback.failed(t); return; } @@ -523,10 +524,10 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio { synchronized(flusher) { - boolean closeConnection = channelState.onOutgoingFrame(frame); if (LOG.isDebugEnabled()) - LOG.debug("sendFrame({}, {}, {}) {}", frame, callback, batch, closeConnection); + LOG.debug("sendFrame({}, {}, {})", frame, callback, batch); + boolean closeConnection = channelState.onOutgoingFrame(frame); if (closeConnection) { Throwable cause = AbnormalCloseStatus.getCause(CloseStatus.getCloseStatus(frame)); @@ -546,6 +547,9 @@ public class WebSocketChannel implements IncomingFrames, FrameHandler.CoreSessio } catch (Throwable t) { + if (LOG.isDebugEnabled()) + LOG.debug("Failed sendFrame()", t); + if (frame.getOpCode() == OpCode.CLOSE) { CloseStatus closeStatus = CloseStatus.getCloseStatus(frame); diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketCloseTest.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketCloseTest.java index 827b1d3a192..3cabfc338c9 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketCloseTest.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketCloseTest.java @@ -19,6 +19,7 @@ package org.eclipse.jetty.websocket.core; import java.net.Socket; +import java.time.Duration; import java.util.concurrent.BlockingQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -52,6 +53,7 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -315,21 +317,22 @@ public class WebSocketCloseTest extends WebSocketTester client.close(); assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - while(true) - { - if (!server.isOpen()) - break; - - server.sendFrame(new Frame(OpCode.TEXT, BufferUtil.toBuffer("frame after close")), Callback.NOOP); - } + assertTimeoutPreemptively(Duration.ofSeconds(1), ()->{ + while(true) + { + if (!server.isOpen()) + break; + server.sendFrame(new Frame(OpCode.TEXT, BufferUtil.toBuffer("frame after close")), Callback.NOOP); + Thread.sleep(100); + } + }); assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); assertNotNull(server.handler.error); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); Callback callback = server.handler.receivedCallback.poll(5, TimeUnit.SECONDS); callback.succeeded(); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); } @ParameterizedTest @@ -433,7 +436,7 @@ public class WebSocketCloseTest extends WebSocketTester @Override public void onOpen(CoreSession coreSession) { - LOG.info("onOpen {}", coreSession); + LOG.debug("onOpen {}", coreSession); session = coreSession; state = session.toString(); opened.countDown(); @@ -442,7 +445,7 @@ public class WebSocketCloseTest extends WebSocketTester @Override public void onFrame(Frame frame, Callback callback) { - LOG.info("onFrame: " + BufferUtil.toDetailString(frame.getPayload())); + LOG.debug("onFrame: " + BufferUtil.toDetailString(frame.getPayload())); state = session.toString(); receivedCallback.offer(callback); receivedFrames.offer(Frame.copy(frame)); @@ -454,7 +457,7 @@ public class WebSocketCloseTest extends WebSocketTester @Override public void onClosed(CloseStatus closeStatus) { - LOG.info("onClosed {}", closeStatus); + LOG.debug("onClosed {}", closeStatus); state = session.toString(); this.closeStatus = closeStatus; closed.countDown(); @@ -463,7 +466,7 @@ public class WebSocketCloseTest extends WebSocketTester @Override public void onError(Throwable cause) { - LOG.info("onError {} ", cause == null?null:cause.toString()); + LOG.debug("onError {} ", cause); error = cause; state = session.toString(); }