Issue #4747 - Throwing in onClose now calls onError

- This applies to the javax onClose method not the core onClosed method.
- Also fixes a bug where onClosed may never be called if onFrame throws.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/CloseStatus.java

@@ -55,7 +55,7 @@ public class CloseStatus
 
     private final int code;
     private final String reason;
-    private final Throwable cause;
+    private Throwable cause;
 
     /**
      * Creates a reason for closing a web socket connection with the no given status code.
@@ -211,6 +211,11 @@ public boolean isAbnormal()
         return !isOrdinary(code);
     }
 
+    public void initCause(Throwable cause)
+    {
+        this.cause = cause;
+    }
+
     public Throwable getCause()
     {
         return cause;

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketCoreSession.java

@@ -679,6 +679,7 @@ private class IncomingAdaptor implements IncomingFrames
         @Override
         public void onFrame(Frame frame, final Callback callback)
         {
+            Callback closeCallback = null;
             try
             {
                 if (LOG.isDebugEnabled())
@@ -695,11 +696,13 @@ public void onFrame(Frame frame, final Callback callback)
 
                 // Handle inbound CLOSE
                 connection.cancelDemand();
-                Callback closeCallback;
-
                 if (closeConnection)
                 {
-                    closeCallback = Callback.from(() -> closeConnection(sessionState.getCloseStatus(), callback));
+                    closeCallback = Callback.from(() -> closeConnection(sessionState.getCloseStatus(), callback), t ->
+                    {
+                        sessionState.onError(t);
+                        closeConnection(sessionState.getCloseStatus(), callback);
+                    });
                 }
                 else
                 {
@@ -725,7 +728,10 @@ public void onFrame(Frame frame, final Callback callback)
             }
             catch (Throwable t)
             {
-                callback.failed(t);
+                if (closeCallback != null)
+                    closeCallback.failed(t);
+                else
+                    callback.failed(t);
             }
         }
     }

jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketSessionState.java

@@ -123,6 +123,28 @@ public boolean onClosed(CloseStatus closeStatus)
         }
     }
 
+    /**
+     * This should only be called directly before closing the connection when we are in {@link State#CLOSED} state.
+     * This ensures an abnormal close status and if we have no error in the CloseStatus we will set one.
+     * @param t the error which occurred.
+     */
+    public void onError(Throwable t)
+    {
+        synchronized (this)
+        {
+            if (_sessionState != State.CLOSED || _closeStatus == null)
+                throw new IllegalArgumentException();
+
+            // Override any normal close status.
+            if (!_closeStatus.isAbnormal())
+                _closeStatus = new CloseStatus(CloseStatus.SERVER_ERROR, t);
+
+            // Otherwise set the error if it wasn't already set to notify onError as well as onClose.
+            if (_closeStatus.getCause() == null)
+                _closeStatus.initCause(t);
+        }
+    }
+
     public boolean onEof()
     {
         synchronized (this)

jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketServer.java

@@ -26,13 +26,14 @@
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.server.handler.ContextHandler;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.websocket.core.server.Negotiation;
 import org.eclipse.jetty.websocket.core.server.WebSocketNegotiator;
 import org.eclipse.jetty.websocket.core.server.WebSocketUpgradeHandler;
 
 public class WebSocketServer
 {
-    private final Server server;
+    private final Server server = new Server();
     private URI serverUri;
 
     public void start() throws Exception
@@ -58,13 +59,26 @@ public Server getServer()
 
     public WebSocketServer(FrameHandler frameHandler)
     {
-        this(new DefaultNegotiator(frameHandler));
+        this(new DefaultNegotiator(frameHandler), false);
     }
 
     public WebSocketServer(WebSocketNegotiator negotiator)
     {
-        server = new Server();
-        ServerConnector connector = new ServerConnector(server);
+        this(negotiator, false);
+    }
+
+    public WebSocketServer(FrameHandler frameHandler, boolean tls)
+    {
+        this(new DefaultNegotiator(frameHandler), tls);
+    }
+
+    public WebSocketServer(WebSocketNegotiator negotiator, boolean tls)
+    {
+        ServerConnector connector;
+        if (tls)
+            connector = new ServerConnector(server, createServerSslContextFactory());
+        else
+            connector = new ServerConnector(server);
         server.addConnector(connector);
 
         ContextHandler context = new ContextHandler("/");
@@ -74,6 +88,14 @@ public WebSocketServer(WebSocketNegotiator negotiator)
         context.setHandler(upgradeHandler);
     }
 
+    private SslContextFactory.Server createServerSslContextFactory()
+    {
+        SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
+        sslContextFactory.setKeyStorePath("src/test/resources/keystore.p12");
+        sslContextFactory.setKeyStorePassword("storepwd");
+        return sslContextFactory;
+    }
+
     public URI getUri()
     {
         return serverUri;

jetty-websocket/websocket-core/src/test/resources/jetty-logging.properties

@@ -1,5 +1,5 @@
 # Jetty Logging using jetty-slf4j-impl
-# org.eclipse.jetty.LEVEL=DEBUG
+org.eclipse.jetty.LEVEL=INFO
 # org.eclipse.jetty.io.LEVEL=DEBUG
 # org.eclipse.jetty.websocket.core.LEVEL=DEBUG
 # org.eclipse.jetty.websocket.core.TestFrameHandler.LEVEL=DEBUG

jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxOnCloseTest.java

@@ -188,27 +188,47 @@ public void abnormalStatusDoesNotChange() throws Exception
         assertThat(clientEndpoint.closeReason.getReasonPhrase(), is("abnormal close 1"));
     }
 
+    @ClientEndpoint
+    public class ThrowOnCloseSocket extends EventSocket
+    {
+        @Override
+        public void onClose(CloseReason reason)
+        {
+            super.onClose(reason);
+            throw new RuntimeException("trigger onError from client onClose");
+        }
+    }
+
     @Test
     public void onErrorOccurringAfterOnClose() throws Exception
     {
-        EventSocket clientEndpoint = new EventSocket();
+        EventSocket clientEndpoint = new ThrowOnCloseSocket();
         URI uri = new URI("ws://localhost:" + connector.getLocalPort() + "/");
         client.connectToServer(clientEndpoint, uri);
 
         OnCloseEndpoint serverEndpoint = Objects.requireNonNull(serverEndpoints.poll(5, TimeUnit.SECONDS));
         assertTrue(serverEndpoint.openLatch.await(5, TimeUnit.SECONDS));
         serverEndpoint.setOnClose((session) ->
         {
-            throw new RuntimeException("trigger onError from onClose");
+            throw new RuntimeException("trigger onError from server onClose");
         });
 
+        // Initiate close on client to cause the server to throw in onClose.
         clientEndpoint.session.close();
-        assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
-        assertThat(clientEndpoint.closeReason.getCloseCode(), is(CloseCodes.UNEXPECTED_CONDITION));
-        assertThat(clientEndpoint.closeReason.getReasonPhrase(), containsString("trigger onError from onClose"));
 
+        // Test the receives the normal close, and throws in onClose.
+        assertTrue(serverEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
+        assertThat(serverEndpoint.closeReason.getCloseCode(), is(CloseCodes.NORMAL_CLOSURE));
         assertTrue(serverEndpoint.errorLatch.await(5, TimeUnit.SECONDS));
         assertThat(serverEndpoint.error, instanceOf(RuntimeException.class));
-        assertThat(serverEndpoint.error.getMessage(), containsString("trigger onError from onClose"));
+        assertThat(serverEndpoint.error.getMessage(), containsString("trigger onError from server onClose"));
+
+
+        assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS));
+        assertThat(clientEndpoint.closeReason.getCloseCode(), is(CloseCodes.UNEXPECTED_CONDITION));
+        assertThat(clientEndpoint.closeReason.getReasonPhrase(), containsString("trigger onError from server onClose"));
+        assertTrue(clientEndpoint.errorLatch.await(5, TimeUnit.SECONDS));
+        assertThat(clientEndpoint.error, instanceOf(RuntimeException.class));
+        assertThat(clientEndpoint.error.getMessage(), containsString("trigger onError from client onClose"));
     }
 }

jetty-websocket/websocket-javax-tests/src/test/resources/jetty-logging.properties

@@ -1,5 +1,5 @@
 # Jetty Logging using jetty-slf4j-impl
-# org.eclipse.jetty.LEVEL=DEBUG
+org.eclipse.jetty.LEVEL=INFO
 # org.eclipse.jetty.util.log.stderr.LONG=true
 # org.eclipse.jetty.server.AbstractConnector.LEVEL=DEBUG
 # org.eclipse.jetty.io.WriteFlusher.LEVEL=DEBUG