From bd50e2941c72c5591e0d52db95d460bd060a4806 Mon Sep 17 00:00:00 2001 From: lachan-roberts Date: Fri, 22 Feb 2019 16:36:19 +1100 Subject: [PATCH] Issue #3170 - changes from review Signed-off-by: lachan-roberts --- .../jetty/websocket/core/FrameHandler.java | 9 +- .../core/proxy/BasicFrameHandler.java | 10 -- .../websocket/core/proxy/WebSocketProxy.java | 54 ++++++----- .../core/proxy/WebSocketProxyTest.java | 94 +++++++++---------- 4 files changed, 77 insertions(+), 90 deletions(-) diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/FrameHandler.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/FrameHandler.java index 6e3d2ad6dc7..885996efa55 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/FrameHandler.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/core/FrameHandler.java @@ -69,8 +69,8 @@ public interface FrameHandler extends IncomingFrames * FrameHandler can write during this call, but can not receive frames until the callback is succeeded. *

*

- * If the FrameHandler succeeds the callback we transition to OPEN state and can now receive frames if not - * auto-demanding or call {@link CoreSession#demand(long)} if demanding. + * If the FrameHandler succeeds the callback we transition to OPEN state and can now receive frames if + * not demanding or can now call {@link CoreSession#demand(long)} to receive frames if demanding. * If the FrameHandler fails the callback a close frame will be sent with {@link CloseStatus#SERVER_ERROR} and * the connection will be closed.
*

@@ -85,9 +85,8 @@ public interface FrameHandler extends IncomingFrames * sequentially to satisfy all outstanding demand signaled by calls to * {@link CoreSession#demand(long)}. * Control and Data frames are passed to this method. - * Control frames that require a response (eg PING and CLOSE) may be responded to by the - * the handler, but if an appropriate response is not sent once the callback is succeeded, - * then a response will be generated and sent. + * Close frames may be responded to by the handler, but if an appropriate close response is not + * sent once the callback is succeeded, then a response close will be generated and sent. * * @param frame the raw frame * @param callback the callback to indicate success in processing frame (or failure) diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/BasicFrameHandler.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/BasicFrameHandler.java index 2387bc113ca..3f0ddee78c8 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/BasicFrameHandler.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/BasicFrameHandler.java @@ -107,18 +107,12 @@ class BasicFrameHandler implements FrameHandler public static class ServerEchoHandler extends BasicFrameHandler { private boolean throwOnFrame; - private boolean noResponse; public void throwOnFrame() { throwOnFrame = true; } - public void noResponseOnFrame() - { - noResponse = true; - } - public ServerEchoHandler(String name) { super(name); @@ -133,9 +127,6 @@ class BasicFrameHandler implements FrameHandler if (throwOnFrame) throw new RuntimeException("intentionally throwing in server onFrame()"); - if (noResponse) - return; - if (frame.isDataFrame()) { System.err.println(name + " echoDataFrame(): " + frame); @@ -145,7 +136,6 @@ class BasicFrameHandler implements FrameHandler { callback.succeeded(); } - } } } \ No newline at end of file diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java index ba95dd30293..e30b4df791a 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java @@ -38,8 +38,8 @@ class WebSocketProxy NOT_OPEN, CONNECTING, OPEN, - ICLOSED, - OCLOSED, + ISHUT, + OSHUT, CLOSED, FAILED } @@ -107,7 +107,6 @@ class WebSocketProxy { System.err.println(toString() + " onOpenSuccess()"); - boolean failServer2Proxy = false; Throwable failure = null; synchronized (lock) { @@ -119,7 +118,6 @@ class WebSocketProxy case FAILED: failure = error; - failServer2Proxy = true; break; default: @@ -128,10 +126,8 @@ class WebSocketProxy } } - if (failServer2Proxy) + if (failure != null) server2Proxy.fail(failure, callback); - else if (failure != null) - callback.failed(failure); else callback.succeeded(); } @@ -152,6 +148,7 @@ class WebSocketProxy case FAILED: failure = error; + failure.addSuppressed(t); break; default: @@ -178,13 +175,15 @@ class WebSocketProxy case OPEN: if (frame.getOpCode() == OpCode.CLOSE) { - state = State.ICLOSED; + state = State.ISHUT; + // the callback is saved until a close response comes in sendFrame from Server2Proxy + // if the callback was completed here then core would send its own close response closeCallback = callback; sendCallback = Callback.from(()->{}, callback::failed); } break; - case OCLOSED: + case OSHUT: if (frame.getOpCode() == OpCode.CLOSE) state = State.CLOSED; break; @@ -248,7 +247,7 @@ class WebSocketProxy sendCallback = Callback.from(callback, failure); break; - case ICLOSED: + case ISHUT: state = State.FAILED; Callback doubleCallback = Callback.from(callback, closeCallback); sendCallback = Callback.from(doubleCallback, failure); @@ -286,10 +285,10 @@ class WebSocketProxy { case OPEN: if (frame.getOpCode() == OpCode.CLOSE) - state = State.OCLOSED; + state = State.OSHUT; break; - case ICLOSED: + case ISHUT: if (frame.getOpCode() == OpCode.CLOSE) { state = State.CLOSED; @@ -376,7 +375,9 @@ class WebSocketProxy break; default: - failure = new IllegalStateException(); + state = State.FAILED; + error = new IllegalStateException(); + failure = error; break; } } @@ -389,7 +390,6 @@ class WebSocketProxy { System.err.println(toString() + " onConnectSuccess(): " + s); - Callback sendCallback = null; Throwable failure = null; synchronized (lock) { @@ -400,19 +400,18 @@ class WebSocketProxy case FAILED: failure = error; - sendCallback = Callback.from(callback, failure); break; default: - failure = new IllegalStateException(); + state = State.FAILED; + error = new IllegalStateException(); + failure = error; break; } } - if (sendCallback != null) - s.close(CloseStatus.SHUTDOWN, failure.getMessage(), sendCallback); - else if (failure != null) - callback.failed(failure); + if (failure != null) + s.close(CloseStatus.SHUTDOWN, failure.getMessage(), Callback.from(callback, failure)); else callback.succeeded(); } @@ -436,10 +435,13 @@ class WebSocketProxy break; default: - failure = new IllegalStateException(); + state = State.FAILED; + error = new IllegalStateException(); + failure = error; break; } } + callback.failed(failure); } @@ -489,13 +491,13 @@ class WebSocketProxy case OPEN: if (frame.getOpCode() == OpCode.CLOSE) { - state = State.ICLOSED; + state = State.ISHUT; closeCallback = callback; sendCallback = Callback.from(()->{}, callback::failed); } break; - case OCLOSED: + case OSHUT: if (frame.getOpCode() == OpCode.CLOSE) state = State.CLOSED; break; @@ -567,7 +569,7 @@ class WebSocketProxy sendCallback = Callback.from(callback, failure); break; - case ICLOSED: + case ISHUT: state = State.FAILED; Callback doubleCallback = Callback.from(callback, closeCallback); sendCallback = Callback.from(doubleCallback, failure); @@ -596,10 +598,10 @@ class WebSocketProxy { case OPEN: if (frame.getOpCode() == OpCode.CLOSE) - state = State.OCLOSED; + state = State.OSHUT; break; - case ICLOSED: + case ISHUT: if (frame.getOpCode() == OpCode.CLOSE) { state = State.CLOSED; diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java index 7e5c04fe8b9..e9411c341c5 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java @@ -153,7 +153,7 @@ public class WebSocketProxyTest WebSocketProxy.Client2Proxy proxyClientSide = proxy.client2Proxy; WebSocketProxy.Server2Proxy proxyServerSide = proxy.server2Proxy; - ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/a"), clientHandler); + ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/"), clientHandler); CompletableFuture response = _client.connect(upgradeRequest); response.get(5, TimeUnit.SECONDS); clientHandler.sendText("hello world"); @@ -264,7 +264,7 @@ public class WebSocketProxyTest WebSocketProxy.Server2Proxy proxyServerSide = proxy.server2Proxy; BasicFrameHandler clientHandler = new BasicFrameHandler("CLIENT"); - ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/test"), clientHandler); + ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/"), clientHandler); CompletableFuture response = _client.connect(upgradeRequest); response.get(5, TimeUnit.SECONDS); @@ -276,40 +276,41 @@ public class WebSocketProxyTest CloseStatus closeStatus; Frame frame; - // Client - frame = clientHandler.receivedFrames.poll(); - assertThat(CloseStatus.getCloseStatus(frame).getCode(), is(CloseStatus.SERVER_ERROR)); - // Client2Proxy frame = proxyClientSide.receivedFrames.poll(); assertThat(frame.getOpCode(), is(OpCode.TEXT)); assertThat(frame.getPayloadAsUTF8(), is("hello world")); - frame = proxyClientSide.receivedFrames.poll(); - closeStatus = CloseStatus.getCloseStatus(frame); - assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); - - frame = proxyClientSide.receivedFrames.poll(); - assertNull(frame); - assertThat(proxyClientSide.getState(), is(WebSocketProxy.State.CLOSED)); - - // Server2Proxy - frame = proxyServerSide.receivedFrames.poll(); - closeStatus = CloseStatus.getCloseStatus(frame); - assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); - - frame = proxyServerSide.receivedFrames.poll(); - assertNull(frame); - assertThat(proxyServerSide.getState(), is(WebSocketProxy.State.CLOSED)); - // Server frame = serverFrameHandler.receivedFrames.poll(); assertThat(frame.getOpCode(), is(OpCode.TEXT)); assertThat(frame.getPayloadAsUTF8(), is("hello world")); frame = serverFrameHandler.receivedFrames.poll(); assertNull(frame); + + // Server2Proxy + frame = proxyServerSide.receivedFrames.poll(); + closeStatus = CloseStatus.getCloseStatus(frame); + assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); + + // Client + frame = clientHandler.receivedFrames.poll(); + closeStatus = CloseStatus.getCloseStatus(frame); + assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); + + // Client2Proxy receiving close response from Client + frame = proxyClientSide.receivedFrames.poll(); + closeStatus = CloseStatus.getCloseStatus(frame); + assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); + + // Check Proxy is in expected final state + assertNull(proxyClientSide.receivedFrames.poll()); + assertNull(proxyServerSide.receivedFrames.poll()); + assertThat(proxyClientSide.getState(), is(WebSocketProxy.State.CLOSED)); + assertThat(proxyServerSide.getState(), is(WebSocketProxy.State.CLOSED)); } @Test @@ -328,13 +329,10 @@ public class WebSocketProxyTest receivedFrames.offer(Frame.copy(frame)); } }; - ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/test"), clientHandler); - CompletableFuture response = _client.connect(upgradeRequest); + CompletableFuture response = _client.connect(clientHandler, new URI("ws://localhost:8080/proxy/")); response.get(5, TimeUnit.SECONDS); - clientHandler.sendText("hello world"); - clientHandler.awaitClose(); serverFrameHandler.awaitClose(); awaitProxyClose(proxyClientSide, proxyServerSide); @@ -342,22 +340,16 @@ public class WebSocketProxyTest CloseStatus closeStatus; Frame frame; - // Client - frame = clientHandler.receivedFrames.poll(); - closeStatus = CloseStatus.getCloseStatus(frame); - assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); - assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); - frame = clientHandler.receivedFrames.poll(); - assertNull(frame); - // Client2Proxy frame = proxyClientSide.receivedFrames.poll(); assertThat(frame.getOpCode(), is(OpCode.TEXT)); assertThat(frame.getPayloadAsUTF8(), is("hello world")); - frame = proxyClientSide.receivedFrames.poll(); - assertNull(frame); - assertThat(proxyClientSide.getState(), is(WebSocketProxy.State.FAILED)); + // Server + frame = serverFrameHandler.receivedFrames.poll(); + assertThat(frame.getOpCode(), is(OpCode.TEXT)); + assertThat(frame.getPayloadAsUTF8(), is("hello world")); + assertNull(serverFrameHandler.receivedFrames.poll()); // Server2Proxy frame = proxyServerSide.receivedFrames.poll(); @@ -365,15 +357,19 @@ public class WebSocketProxyTest assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); - frame = proxyServerSide.receivedFrames.poll(); - assertNull(frame); - assertThat(proxyServerSide.getState(), is(WebSocketProxy.State.FAILED)); + // Client + frame = clientHandler.receivedFrames.poll(); + closeStatus = CloseStatus.getCloseStatus(frame); + assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR)); + assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()")); + assertNull(clientHandler.receivedFrames.poll()); - // Server - frame = serverFrameHandler.receivedFrames.poll(); - assertThat(frame.getOpCode(), is(OpCode.TEXT)); - assertThat(frame.getPayloadAsUTF8(), is("hello world")); - frame = serverFrameHandler.receivedFrames.poll(); - assertNull(frame); + // Client2Proxy does NOT receive close response from the client and fails + assertNull(proxyClientSide.receivedFrames.poll()); + assertThat(proxyClientSide.getState(), is(WebSocketProxy.State.FAILED)); + + // Server2Proxy is failed by the Client2Proxy + assertNull(proxyServerSide.receivedFrames.poll()); + assertThat(proxyServerSide.getState(), is(WebSocketProxy.State.FAILED)); } }