Issue #3734 - throw ISE for WebSocket suspend after close (jetty-9.4) (#4098)

* Issue #3734 - throw ISE for WebSocket suspend after close

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

* Issue #3734 - suspend is error if onClose() has been called

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

jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/SuspendResumeTest.java

@@ -41,6 +41,7 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class SuspendResumeTest
@@ -195,8 +196,7 @@ public class SuspendResumeTest
         assertThat(clientSocket.closeCode, is(StatusCode.NORMAL));
         assertThat(serverSocket.closeCode, is(StatusCode.NORMAL));
 
-        // suspend the client so that no read events occur
+        // suspend after closed throws ISE
-        SuspendToken suspendToken = clientSocket.session.suspend();
+        assertThrows(IllegalStateException.class, () -> clientSocket.session.suspend());
-        suspendToken.resume();
     }
 }

jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java

@@ -68,7 +68,7 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Rem
     private final EventDriver websocket;
     private final Executor executor;
     private final WebSocketPolicy policy;
-    private final AtomicBoolean closed = new AtomicBoolean();
+    private final AtomicBoolean onCloseCalled = new AtomicBoolean(false);
     private ClassLoader classLoader;
     private ExtensionFactory extensionFactory;
     private RemoteEndpointFactory remoteEndpointFactory;
@@ -80,7 +80,6 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Rem
     private UpgradeRequest upgradeRequest;
     private UpgradeResponse upgradeResponse;
     private CompletableFuture<Session> openFuture;
-    private AtomicBoolean onCloseCalled = new AtomicBoolean(false);
 
     public WebSocketSession(WebSocketContainerScope containerScope, URI requestURI, EventDriver websocket, LogicalConnection connection)
     {
@@ -338,10 +337,9 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Rem
     public boolean isOpen()
     {
         if (this.connection == null)
-        {
             return false;
-        }
+
-        return !closed.get() && this.connection.isOpen();
+        return !onCloseCalled.get() && this.connection.isOpen();
     }
 
     @Override
@@ -546,6 +544,9 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Rem
     @Override
     public SuspendToken suspend()
     {
+        if (onCloseCalled.get())
+            throw new IllegalStateException("Not open");
+
         return connection.suspend();
     }
 

jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/ReadState.java

@@ -87,10 +87,8 @@ class ReadState
 
     /**
      * Requests that reads from the connection be suspended.
-     *
-     * @return whether the suspending was successful
      */
-    boolean suspending()
+    void suspending()
     {
         synchronized (this)
         {
@@ -101,9 +99,7 @@ class ReadState
             {
                 case READING:
                     state = State.SUSPENDING;
-                    return true;
+                    break;
-                case EOF:
-                    return false;
                 default:
                     throw new IllegalStateException(toString(state));
             }
@@ -131,8 +127,6 @@ class ReadState
                     ByteBuffer bb = buffer;
                     buffer = null;
                     return bb;
-                case EOF:
-                    return null;
                 default:
                     throw new IllegalStateException(toString(state));
             }

jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/ReadStateTest.java

@@ -27,7 +27,6 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ReadStateTest
 {
@@ -50,7 +49,7 @@ public class ReadStateTest
         ReadState readState = new ReadState();
         assertThat("Initially reading", readState.isReading(), is(true));
 
-        assertTrue(readState.suspending());
+        readState.suspending();
         assertThat("Suspending doesn't take effect immediately", readState.isSuspended(), is(false));
 
         assertNull(readState.resume());
@@ -64,7 +63,7 @@ public class ReadStateTest
         ReadState readState = new ReadState();
         assertThat("Initially reading", readState.isReading(), is(true));
 
-        assertThat(readState.suspending(), is(true));
+        readState.suspending();
         assertThat("Suspending doesn't take effect immediately", readState.isSuspended(), is(false));
 
         ByteBuffer content = BufferUtil.toBuffer("content");
@@ -84,8 +83,8 @@ public class ReadStateTest
 
         assertThat(readState.isReading(), is(false));
         assertThat(readState.isSuspended(), is(true));
-        assertThat(readState.suspending(), is(false));
+        assertThrows(IllegalStateException.class, readState::suspending);
         assertThat(readState.getAction(content), is(ReadState.Action.EOF));
-        assertNull(readState.resume());
+        assertThrows(IllegalStateException.class, readState::resume);
     }
 }