diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java index c37918952fe0..3583989e71dd 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java @@ -409,8 +409,6 @@ else if (values.length == 1) // Verify the negotiated subprotocol List offeredSubProtocols = getSubProtocols(); - if (negotiatedSubProtocol == null && !offeredSubProtocols.isEmpty()) - throw new WebSocketException("Upgrade failed: no subprotocol selected from offered subprotocols "); if (negotiatedSubProtocol != null && !offeredSubProtocols.contains(negotiatedSubProtocol)) throw new WebSocketException("Upgrade failed: subprotocol [" + negotiatedSubProtocol + "] not found in offered subprotocols " + offeredSubProtocols); diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketCoreSession.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketCoreSession.java index 984269d4a652..53de09e21919 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketCoreSession.java +++ b/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); } } } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketSessionState.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketSessionState.java index 1ccf71df9f3e..6054ab8068f3 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketSessionState.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/internal/WebSocketSessionState.java @@ -20,6 +20,7 @@ import java.nio.channels.ClosedChannelException; +import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.websocket.core.CloseStatus; import org.eclipse.jetty.websocket.core.Frame; import org.eclipse.jetty.websocket.core.OpCode; @@ -123,6 +124,39 @@ public boolean onClosed(CloseStatus closeStatus) } } + /** + *

+ * If no error is set in the CloseStatus this will either, replace the current close status with + * a {@link CloseStatus#SERVER_ERROR} status if we had a NORMAL close code, or, it will set the cause + * of the CloseStatus if the previous cause was null, this allows onError to be notified after the connection is closed. + *

+ *

+ * This should only be called if there is an error directly before the call to + * {@link WebSocketCoreSession#closeConnection(CloseStatus, Callback)}. + *

+ *

+ * This could occur if the FrameHandler throws an exception in onFrame after receiving a close frame reply, in this + * case to notify onError we must set the cause in the closeStatus. + *

+ * @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 = new CloseStatus(_closeStatus.getCode(), _closeStatus.getReason(), t); + } + } + public boolean onEof() { synchronized (this) diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java index afd98b662d89..1a0d84c1c5be 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/server/internal/AbstractHandshaker.java @@ -96,7 +96,7 @@ public boolean upgradeRequest(WebSocketNegotiator negotiator, HttpServletRequest return false; } - // Validate negotiated protocol + // Validate negotiated protocol. String protocol = negotiation.getSubprotocol(); List offeredProtocols = negotiation.getOfferedSubprotocols(); if (protocol != null) @@ -104,11 +104,6 @@ public boolean upgradeRequest(WebSocketNegotiator negotiator, HttpServletRequest if (!offeredProtocols.contains(protocol)) throw new WebSocketException("not upgraded: selected a protocol not present in offered protocols"); } - else - { - if (!offeredProtocols.isEmpty()) - throw new WebSocketException("not upgraded: no protocol selected from offered protocols"); - } // validate negotiated extensions for (ExtensionConfig config : negotiation.getNegotiatedExtensions()) 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 334b39842387..bac1e2451d69 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 @@ -28,20 +28,10 @@ import java.util.concurrent.TimeUnit; import org.eclipse.jetty.logging.StacklessLogging; -import org.eclipse.jetty.server.NetworkConnector; -import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.server.ServerConnector; -import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; -import org.eclipse.jetty.util.component.AbstractLifeCycle; -import org.eclipse.jetty.util.ssl.SslContextFactory; -import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession; -import org.eclipse.jetty.websocket.core.server.WebSocketNegotiator; -import org.eclipse.jetty.websocket.core.server.WebSocketUpgradeHandler; -import org.eclipse.jetty.websocket.core.server.internal.RFC6455Handshaker; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -49,6 +39,8 @@ import org.slf4j.LoggerFactory; import static org.eclipse.jetty.util.Callback.NOOP; +import static org.eclipse.jetty.websocket.core.OpCode.CLOSE; +import static org.eclipse.jetty.websocket.core.OpCode.TEXT; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; @@ -71,6 +63,7 @@ public class WebSocketCloseTest extends WebSocketTester private static final String WSS_SCHEME = "wss"; private WebSocketServer server; + private DemandingTestFrameHandler serverHandler; private Socket client; enum State @@ -102,17 +95,16 @@ public void setup(State state, String scheme) throws Exception throw new IllegalStateException(); } - DemandingTestFrameHandler serverHandler = new DemandingTestFrameHandler(); - server = new WebSocketServer(0, serverHandler, tls); + serverHandler = new DemandingTestFrameHandler(); + server = new WebSocketServer(serverHandler, tls); + server.start(); client = newClient(server.getLocalPort(), tls); - assertTrue(server.handler.opened.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.state, containsString("CONNECTED")); - while (true) + assertTrue(serverHandler.opened.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.state, containsString("CONNECTED")); + while (!serverHandler.coreSession.toString().contains("OPEN")) { Thread.yield(); - if (server.handler.getCoreSession().toString().contains("OPEN")) - break; } switch (state) @@ -125,19 +117,19 @@ public void setup(State state, String scheme) throws Exception case ISHUT: { client.getOutputStream().write(RawFrameBuilder.buildClose(new CloseStatus(CloseStatus.NORMAL), true)); - server.handler.getCoreSession().demand(1); + serverHandler.coreSession.demand(1); Frame frame = serverHandler.receivedFrames.poll(5, TimeUnit.SECONDS); assertThat(new CloseStatus(frame.getPayload()).getCode(), is(CloseStatus.NORMAL)); - assertThat(server.handler.getCoreSession().toString(), containsString("ISHUT")); + assertThat(serverHandler.coreSession.toString(), containsString("ISHUT")); LOG.info("Server: ISHUT"); break; } case OSHUT: { - server.sendFrame(CloseStatus.toFrame(CloseStatus.NORMAL)); + serverHandler.coreSession.sendFrame(CloseStatus.toFrame(CloseStatus.NORMAL), NOOP, false); CloseStatus closeStatus = new CloseStatus(receiveFrame(client.getInputStream())); assertThat(closeStatus.getCode(), is(CloseStatus.NORMAL)); - assertThat(server.handler.getCoreSession().toString(), containsString("OSHUT")); + assertThat(serverHandler.coreSession.toString(), containsString("OSHUT")); LOG.info("Server: OSHUT"); break; } @@ -150,12 +142,12 @@ public void testServerCloseISHUT(String scheme) throws Exception { setup(State.ISHUT, scheme); - server.handler.receivedCallback.poll().succeeded(); + serverHandler.receivedCallback.poll().succeeded(); Frame frame = receiveFrame(client.getInputStream()); assertThat(new CloseStatus(frame.getPayload()).getCode(), is(CloseStatus.NORMAL)); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NORMAL)); } @ParameterizedTest @@ -164,13 +156,13 @@ public void testServerDifferentCloseISHUT(String scheme) throws Exception { setup(State.ISHUT, scheme); - server.sendFrame(CloseStatus.toFrame(CloseStatus.SHUTDOWN)); - server.handler.receivedCallback.poll().succeeded(); + serverHandler.coreSession.sendFrame(CloseStatus.toFrame(CloseStatus.SHUTDOWN), NOOP, false); + serverHandler.receivedCallback.poll().succeeded(); Frame frame = receiveFrame(client.getInputStream()); assertThat(new CloseStatus(frame.getPayload()).getCode(), is(CloseStatus.SHUTDOWN)); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SHUTDOWN)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SHUTDOWN)); } @ParameterizedTest @@ -178,14 +170,14 @@ public void testServerDifferentCloseISHUT(String scheme) throws Exception public void testServerFailCloseISHUT(String scheme) throws Exception { setup(State.ISHUT, scheme); - server.handler.receivedCallback.poll().failed(new Exception("test failure")); + serverHandler.receivedCallback.poll().failed(new Exception("test failure")); CloseStatus closeStatus = new CloseStatus(receiveFrame(client.getInputStream())); assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); assertThat(closeStatus.getReason(), is("test failure")); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); } @ParameterizedTest @@ -195,8 +187,8 @@ public void testClientClosesOutputISHUT(String scheme) throws Exception setup(State.ISHUT, scheme); client.shutdownOutput(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - server.handler.receivedCallback.poll().succeeded(); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); + serverHandler.receivedCallback.poll().succeeded(); CloseStatus closeStatus = new CloseStatus(receiveFrame(client.getInputStream())); assertThat(closeStatus.getCode(), is(CloseStatus.NORMAL)); @@ -207,13 +199,13 @@ public void testClientClosesOutputISHUT(String scheme) throws Exception public void testClientCloseOSHUT(String scheme) throws Exception { setup(State.OSHUT, scheme); - server.handler.getCoreSession().demand(1); + serverHandler.coreSession.demand(1); client.getOutputStream().write(RawFrameBuilder.buildClose(new CloseStatus(CloseStatus.NORMAL), true)); - assertNotNull(server.handler.receivedFrames.poll(10, TimeUnit.SECONDS)); - server.handler.receivedCallback.poll().succeeded(); + assertNotNull(serverHandler.receivedFrames.poll(10, TimeUnit.SECONDS)); + serverHandler.receivedCallback.poll().succeeded(); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NORMAL)); assertNull(receiveFrame(client.getInputStream())); } @@ -223,13 +215,13 @@ public void testClientCloseOSHUT(String scheme) throws Exception public void testClientDifferentCloseOSHUT(String scheme) throws Exception { setup(State.OSHUT, scheme); - server.handler.getCoreSession().demand(1); + serverHandler.coreSession.demand(1); client.getOutputStream().write(RawFrameBuilder.buildClose(new CloseStatus(CloseStatus.BAD_PAYLOAD), true)); - assertNotNull(server.handler.receivedFrames.poll(10, TimeUnit.SECONDS)); - server.handler.receivedCallback.poll().succeeded(); + assertNotNull(serverHandler.receivedFrames.poll(10, TimeUnit.SECONDS)); + serverHandler.receivedCallback.poll().succeeded(); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.BAD_PAYLOAD)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.BAD_PAYLOAD)); assertNull(receiveFrame(client.getInputStream())); } @@ -241,13 +233,13 @@ public void testClientCloseServerFailCloseOSHUT(String scheme) throws Exception try (StacklessLogging ignored = new StacklessLogging(WebSocketCoreSession.class)) { setup(State.OSHUT, scheme); - server.handler.getCoreSession().demand(1); + serverHandler.coreSession.demand(1); client.getOutputStream().write(RawFrameBuilder.buildClose(new CloseStatus(CloseStatus.NORMAL), true)); - assertNotNull(server.handler.receivedFrames.poll(10, TimeUnit.SECONDS)); - server.handler.receivedCallback.poll().failed(new Exception("Test")); + assertNotNull(serverHandler.receivedFrames.poll(10, TimeUnit.SECONDS)); + serverHandler.receivedCallback.poll().failed(new Exception("Test")); - assertTrue(server.handler.closed.await(10, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertTrue(serverHandler.closed.await(10, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); assertNull(receiveFrame(client.getInputStream())); } @@ -260,10 +252,10 @@ public void testClientSendsBadFrameOPEN(String scheme) throws Exception setup(State.OPEN, scheme); client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.PONG, "pong frame not masked", false)); - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); - assertThat(server.handler.closeStatus.getReason(), containsString("Client MUST mask all frames")); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); + assertThat(serverHandler.closeStatus.getReason(), containsString("Client MUST mask all frames")); } @ParameterizedTest @@ -273,10 +265,10 @@ public void testClientSendsBadFrameOSHUT(String scheme) throws Exception setup(State.OSHUT, scheme); client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.PONG, "pong frame not masked", false)); - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); - assertThat(server.handler.closeStatus.getReason(), containsString("Client MUST mask all frames")); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); + assertThat(serverHandler.closeStatus.getReason(), containsString("Client MUST mask all frames")); } @ParameterizedTest @@ -286,8 +278,8 @@ public void testClientSendsBadFrameISHUT(String scheme) throws Exception setup(State.ISHUT, scheme); client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.PONG, "pong frame not masked", false)); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.PROTOCOL)); Frame frame = receiveFrame(client.getInputStream()); assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.PROTOCOL)); @@ -301,12 +293,12 @@ public void testClientHalfCloseISHUT(String scheme) throws Exception setup(State.ISHUT, scheme); client.shutdownOutput(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - Callback callback = server.handler.receivedCallback.poll(5, TimeUnit.SECONDS); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); + Callback callback = serverHandler.receivedCallback.poll(5, TimeUnit.SECONDS); callback.succeeded(); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NORMAL)); Frame frame = receiveFrame(client.getInputStream()); assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.NORMAL)); @@ -320,24 +312,24 @@ public void testClientCloseServerWriteISHUT(String scheme) throws Exception setup(State.ISHUT, scheme); client.close(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); assertTimeoutPreemptively(Duration.ofSeconds(1), () -> { while (true) { - if (!server.isOpen()) + if (!serverHandler.coreSession.isOutputOpen()) break; - server.sendFrame(new Frame(OpCode.TEXT, BufferUtil.toBuffer("frame after close")), Callback.NOOP); + serverHandler.coreSession.sendFrame(new Frame(TEXT, BufferUtil.toBuffer("frame after close")), Callback.NOOP, false); Thread.sleep(100); } }); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertNotNull(server.handler.error); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertNotNull(serverHandler.error); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); - Callback callback = server.handler.receivedCallback.poll(5, TimeUnit.SECONDS); + Callback callback = serverHandler.receivedCallback.poll(5, TimeUnit.SECONDS); callback.succeeded(); } @@ -348,10 +340,10 @@ public void testClientAbortsOPEN(String scheme) throws Exception setup(State.OPEN, scheme); client.close(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); } @ParameterizedTest @@ -361,10 +353,10 @@ public void testClientAbortsOSHUT(String scheme) throws Exception setup(State.OSHUT, scheme); client.close(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NO_CLOSE)); } @ParameterizedTest @@ -374,10 +366,10 @@ public void testClientAbortsISHUT(String scheme) throws Exception setup(State.ISHUT, scheme); client.close(); - assertFalse(server.handler.closed.await(250, TimeUnit.MILLISECONDS)); - server.close(); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertFalse(serverHandler.closed.await(250, TimeUnit.MILLISECONDS)); + serverHandler.coreSession.close(CloseStatus.NORMAL, "", NOOP); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NORMAL)); } @ParameterizedTest @@ -386,16 +378,16 @@ public void testOnFrameThrowsOPEN(String scheme) throws Exception { setup(State.OPEN, scheme); - client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.BINARY, "binary", true)); + client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.TEXT, "throw from onFrame", true)); try (StacklessLogging stacklessLogging = new StacklessLogging(WebSocketCoreSession.class)) { - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); } - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(server.handler.closeStatus.getReason(), containsString("onReceiveFrame throws for binary frames")); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("deliberately throwing from onFrame")); } @ParameterizedTest @@ -404,16 +396,16 @@ public void testOnFrameThrowsOSHUT(String scheme) throws Exception { setup(State.OSHUT, scheme); - client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.BINARY, "binary", true)); + client.getOutputStream().write(RawFrameBuilder.buildFrame(OpCode.TEXT, "throw from onFrame", true)); try (StacklessLogging stacklessLogging = new StacklessLogging(WebSocketCoreSession.class)) { - server.handler.getCoreSession().demand(1); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); } - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(server.handler.closeStatus.getReason(), containsString("onReceiveFrame throws for binary frames")); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("deliberately throwing from onFrame")); } @ParameterizedTest @@ -422,10 +414,10 @@ public void testAbnormalCloseStatusIsHardClose(String scheme) throws Exception { setup(State.OPEN, scheme); - server.handler.getCoreSession().close(CloseStatus.SERVER_ERROR, "manually sent server error", Callback.NOOP); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(server.handler.closeStatus.getReason(), containsString("manually sent server error")); + serverHandler.coreSession.close(CloseStatus.SERVER_ERROR, "manually sent server error", Callback.NOOP); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("manually sent server error")); Frame frame = receiveFrame(client.getInputStream()); assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.SERVER_ERROR)); @@ -438,9 +430,9 @@ public void doubleNormalClose(String scheme) throws Exception setup(State.OPEN, scheme); Callback.Completable callback1 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.NORMAL, "normal 1", callback1); + serverHandler.coreSession.close(CloseStatus.NORMAL, "normal 1", callback1); Callback.Completable callback2 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.NORMAL, "normal 2", callback2); + serverHandler.coreSession.close(CloseStatus.NORMAL, "normal 2", callback2); // First Callback Succeeded assertDoesNotThrow(() -> callback1.get(5, TimeUnit.SECONDS)); @@ -451,7 +443,7 @@ public void doubleNormalClose(String scheme) throws Exception // Normal close frame received on client. Frame closeFrame = receiveFrame(client.getInputStream()); - assertThat(closeFrame.getOpCode(), is(OpCode.CLOSE)); + assertThat(closeFrame.getOpCode(), is(CLOSE)); CloseStatus closeStatus = CloseStatus.getCloseStatus(closeFrame); assertThat(closeStatus.getCode(), is(CloseStatus.NORMAL)); assertThat(closeStatus.getReason(), is("normal 1")); @@ -460,14 +452,14 @@ public void doubleNormalClose(String scheme) throws Exception client.getOutputStream().write(RawFrameBuilder.buildClose( new CloseStatus(CloseStatus.NORMAL, "normal response 1"), true)); - server.handler.getCoreSession().demand(1); - assertNotNull(server.handler.receivedFrames.poll(10, TimeUnit.SECONDS)); - Callback closeFrameCallback = Objects.requireNonNull(server.handler.receivedCallback.poll()); + serverHandler.coreSession.demand(1); + assertNotNull(serverHandler.receivedFrames.poll(10, TimeUnit.SECONDS)); + Callback closeFrameCallback = Objects.requireNonNull(serverHandler.receivedCallback.poll()); closeFrameCallback.succeeded(); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.NORMAL)); - assertThat(server.handler.closeStatus.getReason(), is("normal response 1")); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.NORMAL)); + assertThat(serverHandler.closeStatus.getReason(), is("normal response 1")); } @ParameterizedTest @@ -477,9 +469,9 @@ public void doubleAbnormalClose(String scheme) throws Exception setup(State.OPEN, scheme); Callback.Completable callback1 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.SERVER_ERROR, "server error should succeed", callback1); + serverHandler.coreSession.close(CloseStatus.SERVER_ERROR, "server error should succeed", callback1); Callback.Completable callback2 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.PROTOCOL, "protocol error should fail", callback2); + serverHandler.coreSession.close(CloseStatus.PROTOCOL, "protocol error should fail", callback2); // First Callback Succeeded assertDoesNotThrow(() -> callback1.get(5, TimeUnit.SECONDS)); @@ -488,9 +480,9 @@ public void doubleAbnormalClose(String scheme) throws Exception ExecutionException error = assertThrows(ExecutionException.class, () -> callback2.get(5, TimeUnit.SECONDS)); assertThat(error.getCause(), instanceOf(ClosedChannelException.class)); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(server.handler.closeStatus.getReason(), containsString("server error should succeed")); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("server error should succeed")); Frame frame = receiveFrame(client.getInputStream()); assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.SERVER_ERROR)); @@ -503,9 +495,9 @@ public void doubleCloseAbnormalOvertakesNormalClose(String scheme) throws Except setup(State.OPEN, scheme); Callback.Completable callback1 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.NORMAL, "normal close (client does not complete close handshake)", callback1); + serverHandler.coreSession.close(CloseStatus.NORMAL, "normal close (client does not complete close handshake)", callback1); Callback.Completable callback2 = new Callback.Completable(); - server.handler.getCoreSession().close(CloseStatus.SERVER_ERROR, "error close should overtake normal close", callback2); + serverHandler.coreSession.close(CloseStatus.SERVER_ERROR, "error close should overtake normal close", callback2); // First Callback Succeeded assertDoesNotThrow(() -> callback1.get(5, TimeUnit.SECONDS)); @@ -514,18 +506,33 @@ public void doubleCloseAbnormalOvertakesNormalClose(String scheme) throws Except ExecutionException error = assertThrows(ExecutionException.class, () -> callback2.get(5, TimeUnit.SECONDS)); assertThat(error.getCause(), instanceOf(ClosedChannelException.class)); - assertTrue(server.handler.closed.await(5, TimeUnit.SECONDS)); - assertThat(server.handler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(server.handler.closeStatus.getReason(), containsString("error close should overtake normal close")); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("error close should overtake normal close")); Frame frame = receiveFrame(client.getInputStream()); assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.NORMAL)); } - static class DemandingTestFrameHandler implements SynchronousFrameHandler + @ParameterizedTest + @ValueSource(strings = {WS_SCHEME, WSS_SCHEME}) + public void testThrowFromOnCloseFrame(String scheme) throws Exception + { + setup(State.OSHUT, scheme); + + CloseStatus closeStatus = new CloseStatus(CloseStatus.NORMAL, "throw from onFrame"); + client.getOutputStream().write(RawFrameBuilder.buildClose(closeStatus, true)); + + serverHandler.coreSession.demand(1); + assertTrue(serverHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(serverHandler.closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(serverHandler.closeStatus.getReason(), containsString("deliberately throwing from onFrame")); + } + + private static class DemandingTestFrameHandler implements FrameHandler { private CoreSession coreSession; - String state; + private String state; protected BlockingQueue receivedFrames = new BlockingArrayQueue<>(); protected BlockingQueue receivedCallback = new BlockingArrayQueue<>(); @@ -534,23 +541,19 @@ static class DemandingTestFrameHandler implements SynchronousFrameHandler protected CountDownLatch closed = new CountDownLatch(1); protected CloseStatus closeStatus = null; - public CoreSession getCoreSession() - { - return coreSession; - } - public BlockingQueue getFrames() { return receivedFrames; } @Override - public void onOpen(CoreSession coreSession) + public void onOpen(CoreSession coreSession, Callback callback) { LOG.debug("onOpen {}", coreSession); this.coreSession = coreSession; state = this.coreSession.toString(); opened.countDown(); + callback.succeeded(); } @Override @@ -561,25 +564,29 @@ public void onFrame(Frame frame, Callback callback) receivedCallback.offer(callback); receivedFrames.offer(Frame.copy(frame)); - if (frame.getOpCode() == OpCode.BINARY) - throw new IllegalArgumentException("onReceiveFrame throws for binary frames"); + byte opCode = frame.getOpCode(); + if ((opCode == TEXT && "throw from onFrame".equals(frame.getPayloadAsUTF8())) || + (opCode == CLOSE && "throw from onFrame".equals(CloseStatus.getCloseStatus(frame).getReason()))) + throw new RuntimeException("deliberately throwing from onFrame"); } @Override - public void onClosed(CloseStatus closeStatus) + public void onClosed(CloseStatus closeStatus, Callback callback) { LOG.debug("onClosed {}", closeStatus); state = coreSession.toString(); this.closeStatus = closeStatus; closed.countDown(); + callback.succeeded(); } @Override - public void onError(Throwable cause) + public void onError(Throwable cause, Callback callback) { - LOG.debug("onError {} ", cause); + LOG.debug("onError", cause); error = cause; state = coreSession.toString(); + callback.succeeded(); } @Override @@ -587,101 +594,5 @@ public boolean isDemanding() { return true; } - - public void sendText(String text) - { - Frame frame = new Frame(OpCode.TEXT); - frame.setFin(true); - frame.setPayload(text); - - getCoreSession().sendFrame(frame, NOOP, false); - state = coreSession.toString(); - } - } - - static class WebSocketServer extends AbstractLifeCycle - { - private static Logger LOG = LoggerFactory.getLogger(WebSocketServer.class); - private final Server server; - private final DemandingTestFrameHandler handler; - - public void doStart() throws Exception - { - server.start(); - } - - public void doStop() throws Exception - { - server.stop(); - } - - public int getLocalPort() - { - return server.getBean(NetworkConnector.class).getLocalPort(); - } - - private SslContextFactory.Server createServerSslContextFactory() - { - SslContextFactory.Server sslContextFactory = new SslContextFactory.Server(); - sslContextFactory.setKeyStorePath("src/test/resources/keystore.p12"); - sslContextFactory.setKeyStorePassword("storepwd"); - return sslContextFactory; - } - - public WebSocketServer(int port, DemandingTestFrameHandler frameHandler, boolean tls) - { - this.handler = frameHandler; - server = new Server(); - server.getBean(QueuedThreadPool.class).setName("WSCoreServer"); - - ServerConnector connector; - if (tls) - connector = new ServerConnector(server, createServerSslContextFactory()); - else - connector = new ServerConnector(server); - - connector.addBean(new RFC6455Handshaker()); - connector.setPort(port); - connector.setIdleTimeout(1000000); - server.addConnector(connector); - - ContextHandler context = new ContextHandler("/"); - server.setHandler(context); - WebSocketNegotiator negotiator = new TestWebSocketNegotiator(frameHandler); - - WebSocketUpgradeHandler upgradeHandler = new TestWebSocketUpgradeHandler(negotiator); - context.setHandler(upgradeHandler); - } - - public void sendFrame(Frame frame) - { - handler.getCoreSession().sendFrame(frame, NOOP, false); - } - - public void sendFrame(Frame frame, Callback callback) - { - handler.getCoreSession().sendFrame(frame, callback, false); - } - - public void sendText(String line) - { - LOG.info("sending {}...", line); - handler.sendText(line); - } - - public BlockingQueue getFrames() - { - return handler.getFrames(); - } - - public void close() - { - handler.getCoreSession().close(CloseStatus.NORMAL, "WebSocketServer Initiated Close", Callback.NOOP); - } - - public boolean isOpen() - { - return handler.getCoreSession().isOutputOpen(); - } } } diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java index 5f9e5482426d..237f80126250 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java @@ -317,17 +317,28 @@ public void testSubProtocolNotOffered() throws Exception public void testNoSubProtocolSelected() throws Exception { TestFrameHandler clientHandler = new TestFrameHandler(); - ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(client, server.getUri(), clientHandler); upgradeRequest.setSubProtocols("testNoSubProtocolSelected"); - - try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class)) + CompletableFuture headers = new CompletableFuture<>(); + upgradeRequest.addListener(new UpgradeListener() { - CompletableFuture connect = client.connect(upgradeRequest); - Throwable t = assertThrows(ExecutionException.class, () -> connect.get(5, TimeUnit.SECONDS)); - assertThat(t.getMessage(), containsString("Failed to upgrade to websocket:")); - assertThat(t.getMessage(), containsString("500 Server Error")); - } + @Override + public void onHandshakeResponse(HttpRequest request, HttpResponse response) + { + headers.complete(response.getHeaders()); + } + }); + + CoreSession session = client.connect(upgradeRequest).get(5, TimeUnit.SECONDS); + session.close(Callback.NOOP); + assertTrue(clientHandler.closed.await(5, TimeUnit.SECONDS)); + assertThat(clientHandler.closeStatus.getCode(), is(CloseStatus.NO_CODE)); + + // RFC6455: If the server does not agree to any of the client's requested subprotocols, the only acceptable + // value is null. It MUST NOT send back a |Sec-WebSocket-Protocol| header field in its response. + HttpFields httpFields = headers.get(); + assertThat(httpFields.get(HttpHeader.UPGRADE), is("WebSocket")); + assertNull(httpFields.get(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL)); } @Test diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketServer.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketServer.java index a76cd1057bb9..c74cbf7a490d 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/WebSocketServer.java +++ b/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; diff --git a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxClientUpgradeRequest.java b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxClientUpgradeRequest.java index b2fd4d5ae521..d2c8856db665 100644 --- a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxClientUpgradeRequest.java +++ b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxClientUpgradeRequest.java @@ -64,4 +64,10 @@ public URI getRequestURI() { return getURI(); } + + @Override + public String getPathInContext() + { + throw new UnsupportedOperationException(); + } } diff --git a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientFrameHandlerFactory.java b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientFrameHandlerFactory.java index 39f3221b8772..5a1b5252e0ea 100644 --- a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientFrameHandlerFactory.java +++ b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientFrameHandlerFactory.java @@ -40,7 +40,7 @@ public JavaxWebSocketClientFrameHandlerFactory(JavaxWebSocketContainer container } @Override - public EndpointConfig newDefaultEndpointConfig(Class endpointClass, String path) + public EndpointConfig newDefaultEndpointConfig(Class endpointClass) { return new BasicClientEndpointConfig(); } diff --git a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java index 8ce0df47cdb3..acc50088c95a 100644 --- a/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java +++ b/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java @@ -19,10 +19,10 @@ package org.eclipse.jetty.websocket.javax.client.internal; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.stream.Stream; import javax.websocket.ClientEndpointConfig.Configurator; import javax.websocket.HandshakeResponse; @@ -44,18 +44,15 @@ public JsrUpgradeListener(Configurator configurator) public void onHandshakeRequest(HttpRequest request) { if (configurator == null) - { return; - } HttpFields fields = request.getHeaders(); - Map> originalHeaders = new HashMap<>(); - fields.forEach((field) -> + fields.forEach(field -> { - List values = new ArrayList<>(); - Stream.of(field.getValues()).forEach((val) -> values.add(val)); - originalHeaders.put(field.getName(), values); + originalHeaders.putIfAbsent(field.getName(), new ArrayList<>()); + List values = originalHeaders.get(field.getName()); + Collections.addAll(values, field.getValues()); }); // Give headers to configurator @@ -63,26 +60,23 @@ public void onHandshakeRequest(HttpRequest request) // Reset headers on HttpRequest per configurator fields.clear(); - originalHeaders.forEach((name, values) -> fields.put(name, values)); + originalHeaders.forEach(fields::put); } @Override public void onHandshakeResponse(HttpRequest request, HttpResponse response) { if (configurator == null) - { return; - } HandshakeResponse handshakeResponse = () -> { - HttpFields fields = response.getHeaders(); Map> ret = new HashMap<>(); - fields.forEach((field) -> + response.getHeaders().forEach(field -> { - List values = new ArrayList<>(); - Stream.of(field.getValues()).forEach((val) -> values.add(val)); - ret.put(field.getName(), values); + ret.putIfAbsent(field.getName(), new ArrayList<>()); + List values = ret.get(field.getName()); + Collections.addAll(values, field.getValues()); }); return ret; }; diff --git a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java index 5eeea2518089..eb51e98d4fa8 100644 --- a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java +++ b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java @@ -144,7 +144,7 @@ public JavaxWebSocketFrameHandlerFactory(JavaxWebSocketContainer container, Invo public abstract JavaxWebSocketFrameHandlerMetadata getMetadata(Class endpointClass, EndpointConfig endpointConfig); - public abstract EndpointConfig newDefaultEndpointConfig(Class endpointClass, String path); + public abstract EndpointConfig newDefaultEndpointConfig(Class endpointClass); public JavaxWebSocketFrameHandler newJavaxWebSocketFrameHandler(Object endpointInstance, UpgradeRequest upgradeRequest) { @@ -160,8 +160,7 @@ public JavaxWebSocketFrameHandler newJavaxWebSocketFrameHandler(Object endpointI else { endpoint = endpointInstance; - String path = (upgradeRequest.getRequestURI() == null) ? null : upgradeRequest.getRequestURI().getPath(); - config = newDefaultEndpointConfig(endpoint.getClass(), path); + config = newDefaultEndpointConfig(endpoint.getClass()); } JavaxWebSocketFrameHandlerMetadata metadata = getMetadata(endpoint.getClass(), config); @@ -180,7 +179,7 @@ public JavaxWebSocketFrameHandler newJavaxWebSocketFrameHandler(Object endpointI if (templatePathSpec != null) { String[] namedVariables = templatePathSpec.getVariables(); - Map pathParams = templatePathSpec.getPathParams(upgradeRequest.getRequestURI().getRawPath()); + Map pathParams = templatePathSpec.getPathParams(upgradeRequest.getPathInContext()); // Handle parameterized @PathParam entries openHandle = bindTemplateVariables(openHandle, namedVariables, pathParams); diff --git a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketSession.java b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketSession.java index 9cbe7d6056d6..7c40cd411a2a 100644 --- a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketSession.java +++ b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketSession.java @@ -28,6 +28,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import javax.websocket.CloseReason; @@ -62,6 +63,7 @@ public class JavaxWebSocketSession implements javax.websocket.Session private final AvailableDecoders availableDecoders; private final AvailableEncoders availableEncoders; private final Map pathParameters; + private final String sessionId; private Map userProperties; private List negotiatedExtensions; @@ -76,8 +78,8 @@ public JavaxWebSocketSession(JavaxWebSocketContainer container, this.container = container; this.coreSession = coreSession; this.frameHandler = frameHandler; + this.sessionId = UUID.randomUUID().toString(); this.config = Objects.requireNonNull(endpointConfig); - this.availableDecoders = new AvailableDecoders(this.config); this.availableEncoders = new AvailableEncoders(this.config); @@ -179,7 +181,7 @@ public void addMessageHandler(MessageHandler handler) throws IllegalStateExcepti @Override public void close() { - close(new CloseReason(CloseReason.CloseCodes.NO_STATUS_CODE, null)); + close(new CloseReason(CloseReason.CloseCodes.NORMAL_CLOSURE, null)); } /** @@ -315,7 +317,7 @@ public void abort() @Override public String getId() { - return this.frameHandler.getUpgradeRequest().toString(); + return sessionId; } /** diff --git a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequest.java b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequest.java index 90de5c9c4923..5764910685a8 100644 --- a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequest.java +++ b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequest.java @@ -31,9 +31,14 @@ public interface UpgradeRequest Principal getUserPrincipal(); /** - * For obtaining {@link javax.websocket.server.PathParam} values from Request URI path - * - * @return the request URI + * @return the full URI of this request. */ URI getRequestURI(); + + /** + * For obtaining {@link javax.websocket.server.PathParam} values from the Request context path. + * + * @return the path in Context. + */ + String getPathInContext(); } diff --git a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequestAdapter.java b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequestAdapter.java index 4ab2345d6695..42e2f94e7fab 100644 --- a/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequestAdapter.java +++ b/jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/UpgradeRequestAdapter.java @@ -24,16 +24,18 @@ public class UpgradeRequestAdapter implements UpgradeRequest { private final URI requestURI; + private final String pathInContext; public UpgradeRequestAdapter() { /* anonymous, no requestURI, upgrade request */ - this(null); + this(null, null); } - public UpgradeRequestAdapter(URI uri) + public UpgradeRequestAdapter(URI uri, String pathInContext) { this.requestURI = uri; + this.pathInContext = pathInContext; } @Override @@ -47,4 +49,10 @@ public URI getRequestURI() { return requestURI; } + + @Override + public String getPathInContext() + { + return pathInContext; + } } diff --git a/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/AbstractSessionTest.java b/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/AbstractSessionTest.java index e425420c99b9..c96927c7dc64 100644 --- a/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/AbstractSessionTest.java +++ b/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/AbstractSessionTest.java @@ -41,7 +41,7 @@ public static void initSession() throws Exception JavaxWebSocketFrameHandler frameHandler = container.newFrameHandler(websocketPojo, upgradeRequest); CoreSession coreSession = new CoreSession.Empty(); session = new JavaxWebSocketSession(container, coreSession, frameHandler, container.getFrameHandlerFactory() - .newDefaultEndpointConfig(websocketPojo.getClass(), null)); + .newDefaultEndpointConfig(websocketPojo.getClass())); } @AfterAll diff --git a/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/DummyFrameHandlerFactory.java b/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/DummyFrameHandlerFactory.java index d7468d6a2d0a..12d00d0c67d8 100644 --- a/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/DummyFrameHandlerFactory.java +++ b/jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/DummyFrameHandlerFactory.java @@ -33,7 +33,7 @@ public DummyFrameHandlerFactory(JavaxWebSocketContainer container) } @Override - public EndpointConfig newDefaultEndpointConfig(Class endpointClass, String path) + public EndpointConfig newDefaultEndpointConfig(Class endpointClass) { return ClientEndpointConfig.Builder.create().build(); } diff --git a/jetty-websocket/websocket-javax-server/src/main/java/org/eclipse/jetty/websocket/javax/server/internal/JavaxServerUpgradeRequest.java b/jetty-websocket/websocket-javax-server/src/main/java/org/eclipse/jetty/websocket/javax/server/internal/JavaxServerUpgradeRequest.java index 9b4d4d559393..69624723ec89 100644 --- a/jetty-websocket/websocket-javax-server/src/main/java/org/eclipse/jetty/websocket/javax/server/internal/JavaxServerUpgradeRequest.java +++ b/jetty-websocket/websocket-javax-server/src/main/java/org/eclipse/jetty/websocket/javax/server/internal/JavaxServerUpgradeRequest.java @@ -42,6 +42,12 @@ public Principal getUserPrincipal() @Override public URI getRequestURI() { - return this.servletRequest.getRequestURI(); + return servletRequest.getRequestURI(); + } + + @Override + public String getPathInContext() + { + return servletRequest.getPathInContext(); } } diff --git a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxOnCloseTest.java b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxOnCloseTest.java index 138266304c82..6b02668b7179 100644 --- a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxOnCloseTest.java +++ b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxOnCloseTest.java @@ -188,10 +188,21 @@ 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); @@ -199,16 +210,25 @@ public void onErrorOccurringAfterOnClose() throws Exception 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")); } } diff --git a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JettySpecificConfigTest.java b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JettySpecificConfigTest.java index 4c48c3508987..e1c61dab3a27 100644 --- a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JettySpecificConfigTest.java +++ b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JettySpecificConfigTest.java @@ -146,7 +146,7 @@ public void testJettySpecificConfig() throws Exception // Close the Session. session.close(); assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS)); - assertThat(clientEndpoint.closeReason.getCloseCode(), is(CloseReason.CloseCodes.NO_STATUS_CODE)); + assertThat(clientEndpoint.closeReason.getCloseCode(), is(CloseReason.CloseCodes.NORMAL_CLOSURE)); assertNull(clientEndpoint.error); } } diff --git a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/PathParamTest.java b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/PathParamTest.java index 2b3e28eeff84..b67e44a462aa 100644 --- a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/PathParamTest.java +++ b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/PathParamTest.java @@ -53,7 +53,7 @@ public void startContainer() throws Exception _server.addConnector(_connector); _context = new ServletContextHandler(ServletContextHandler.SESSIONS); - _context.setContextPath("/"); + _context.setContextPath("/context"); _server.setHandler(_context); JavaxWebSocketServletContainerInitializer.configure(_context, (context, container) -> @@ -68,7 +68,7 @@ public void stopContainer() throws Exception _server.stop(); } - @ServerEndpoint("/pathparam/echo/{name}") + @ServerEndpoint("/pathParam/echo/{name}") public static class EchoParamSocket { private Session session; @@ -92,7 +92,7 @@ public void testBasicPathParamSocket() throws Exception WebSocketContainer container = ContainerProvider.getWebSocketContainer(); EventSocket clientEndpoint = new EventSocket(); - URI serverUri = URI.create("ws://localhost:" + _connector.getLocalPort() + "/pathparam/echo/myParam"); + URI serverUri = URI.create("ws://localhost:" + _connector.getLocalPort() + "/context/pathParam/echo/myParam"); Session session = container.connectToServer(clientEndpoint, serverUri); session.getBasicRemote().sendText("echo"); diff --git a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/SyntheticOnMessageTest.java b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/SyntheticOnMessageTest.java new file mode 100644 index 000000000000..8130905ee422 --- /dev/null +++ b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/SyntheticOnMessageTest.java @@ -0,0 +1,112 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under +// the terms of the Eclipse Public License 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0 +// +// This Source Code may also be made available under the following +// Secondary Licenses when the conditions for such availability set +// forth in the Eclipse Public License, v. 2.0 are satisfied: +// the Apache License v2.0 which is available at +// https://www.apache.org/licenses/LICENSE-2.0 +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.websocket.javax.tests; + +import java.lang.reflect.Method; +import java.net.URI; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.websocket.CloseReason; +import javax.websocket.ContainerProvider; +import javax.websocket.OnMessage; +import javax.websocket.Session; +import javax.websocket.WebSocketContainer; +import javax.websocket.server.ServerEndpoint; + +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.component.LifeCycle; +import org.eclipse.jetty.websocket.javax.server.config.JavaxWebSocketServletContainerInitializer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +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.assertTrue; + +public class SyntheticOnMessageTest +{ + private Server server; + private URI serverUri; + private ServerConnector connector; + private WebSocketContainer client; + + @BeforeEach + public void before() throws Exception + { + server = new Server(); + connector = new ServerConnector(server); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + JavaxWebSocketServletContainerInitializer.configure(contextHandler, (context, container) -> + container.addEndpoint(ServerSocket.class)); + server.setHandler(contextHandler); + server.start(); + serverUri = URI.create("ws://localhost:" + connector.getLocalPort()); + client = ContainerProvider.getWebSocketContainer(); + } + + @AfterEach + public void after() throws Exception + { + LifeCycle.stop(client); + server.stop(); + } + + public static class AnnotatedEndpoint + { + public void onMessage(T message) + { + } + } + + @ServerEndpoint("/") + public static class ServerSocket extends AnnotatedEndpoint + { + @OnMessage + public void onMessage(String message) + { + } + } + + @Test + public void syntheticOnMessageTest() throws Exception + { + // ServerSocket has two annotated onMessage methods, one is a synthetic bridge method generated + // by the compiler and shouldn't be used. + List annotatedOnMessages = Stream.of(ServerSocket.class.getDeclaredMethods()) + .filter(method -> method.getAnnotation(OnMessage.class) != null) + .collect(Collectors.toList()); + assertThat(annotatedOnMessages.size(), is(2)); + + // We should correctly filter out all synthetic methods so we should not get an InvalidSignatureException. + EventSocket clientSocket = new EventSocket(); + Session session = client.connectToServer(clientSocket, serverUri); + assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS)); + session.close(); + assertTrue(clientSocket.closeLatch.await(5, TimeUnit.SECONDS)); + assertThat(clientSocket.closeReason.getCloseCode(), is(CloseReason.CloseCodes.NORMAL_CLOSURE)); + } +} diff --git a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/JavaxWebSocketFrameHandlerOnMessageTextStreamTest.java b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/JavaxWebSocketFrameHandlerOnMessageTextStreamTest.java index 781172066728..3b1b6b5abe43 100644 --- a/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/JavaxWebSocketFrameHandlerOnMessageTextStreamTest.java +++ b/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/server/JavaxWebSocketFrameHandlerOnMessageTextStreamTest.java @@ -46,7 +46,8 @@ public class JavaxWebSocketFrameHandlerOnMessageTextStreamTest extends AbstractJ @SuppressWarnings("Duplicates") private T performOnMessageInvocation(T socket, Consumer func) throws Exception { - UpgradeRequest request = new UpgradeRequestAdapter(URI.create("http://localhost:8080/msg/foo")); + URI uri = URI.create("http://localhost:8080/msg/foo"); + UpgradeRequest request = new UpgradeRequestAdapter(uri, uri.getPath()); // Establish endpoint function JavaxWebSocketFrameHandler frameHandler = container.newFrameHandler(socket, request); diff --git a/jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/ServletUpgradeRequest.java b/jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/ServletUpgradeRequest.java index ef81155a00b9..f327a424a958 100644 --- a/jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/ServletUpgradeRequest.java +++ b/jetty-websocket/websocket-servlet/src/main/java/org/eclipse/jetty/websocket/servlet/ServletUpgradeRequest.java @@ -38,6 +38,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.websocket.core.ExtensionConfig; import org.eclipse.jetty.websocket.core.WebSocketConstants; import org.eclipse.jetty.websocket.core.server.Negotiation; @@ -314,6 +315,14 @@ public URI getRequestURI() return requestURI; } + /** + * @return the path within the context, combination of the ServletPath with the PathInfo. + */ + public String getPathInContext() + { + return URIUtil.addPaths(request.getServletPath(), request.getPathInfo()); + } + /** * @param name Attribute name * @return Attribute value or null diff --git a/jetty-websocket/websocket-util/src/main/java/org/eclipse/jetty/websocket/util/ReflectUtils.java b/jetty-websocket/websocket-util/src/main/java/org/eclipse/jetty/websocket/util/ReflectUtils.java index c038b45ead20..bb1a33e6e99d 100644 --- a/jetty-websocket/websocket-util/src/main/java/org/eclipse/jetty/websocket/util/ReflectUtils.java +++ b/jetty-websocket/websocket-util/src/main/java/org/eclipse/jetty/websocket/util/ReflectUtils.java @@ -29,6 +29,7 @@ import java.util.ArrayList; import java.util.List; import java.util.regex.Pattern; +import java.util.stream.Stream; public class ReflectUtils { @@ -220,27 +221,19 @@ public static Method findAnnotatedMethod(Class pojo, Class pojo, Class anno) { - List methods = null; Class clazz = pojo; - + List methods = new ArrayList<>(); while ((clazz != null) && Object.class.isAssignableFrom(clazz)) { - for (Method method : clazz.getDeclaredMethods()) - { - if (method.getAnnotation(anno) != null) - { - if (methods == null) - methods = new ArrayList<>(); - methods.add(method); - } - } + Stream.of(clazz.getDeclaredMethods()) + .filter(method -> !method.isSynthetic() && (method.getAnnotation(anno) != null)) + .forEach(methods::add); clazz = clazz.getSuperclass(); } - if (methods == null) + if (methods.isEmpty()) return null; - int len = methods.size(); - return methods.toArray(new Method[len]); + return methods.toArray(new Method[0]); } /**