Issue #3170 - WebSocket Proxy #3365

lachlan-roberts · 2019-02-15T04:15:02Z

Implementation of a non-blocking WebSocketProxy using websocket-core as a proof of concept for issue #3170.

Added WebSocketProxyTest to test the WebSocketProxy with various failures
Improved javadoc on FrameHandler.
Some cleanups and bug fixes in WebSocketChannel

sbordet · 2019-02-15T08:02:40Z

@lachlan-roberts this is a WIP PR, was that intended?

I see the old proxy, have you decided to go with it rather than then enum+switch based one?

lachlan-roberts · 2019-02-15T08:11:38Z

@sbordet yes i was just trying out the feature of the WIP PR, I will go with the new WebSocketProxy class, I have further changes and new tests for it but I have not pushed them yet, still working out some issues with the tests. Some of the issues with the tests are fixed with the PR #3336, so it would be good to get that merged soon

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

- Introduced an EMPTY_SESSION in the ProxyFrameHandler as a terminal state to know whether a FailedCoreSession has been handled - Use while(true) loops to do the compareAndSet in ProxyFrameHandler - Improved the tests for the proxy so that it tests the frames received at every state (ie Client Proxy and Server) Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

- Redesigned the proxy into a new class called WebSocketProxy containing a Client2Proxy FrameHandler and a Server2Proxy FrameHandler - WebSocketProxy uses synchronized blocks with an enum state instead of the previous compare and sets - Created a new test similar to ProxyFrameHandlerTest to test the new WebSocketProxy Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

…nges only do state changes inside synchronized blocks, remember what action to do and only do this outside of the synchronized block Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

- Added test cases to test failures in and around the WebSocketProxy and how it handles them. - In WebSocketChannel.sendFrame() we were using a null cause for closeConnection, we are now extracting the cause from the AbnormalCloseStatus. This was resulting in onError not being called when there was actually an error and an AbnormalCloseStatus. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

removing old versions of the proxy frame handler adding licence headers Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw

I started this review and made a lot of comments.... and then realised that most were based on a fundamental misunderstanding. So I have left them there for now as an indication of why I don't like the design. I thought there was a single proxy state protected my a synchronised lock... but there are two states protected by one locks? This is really strange as we grab the lock twice and change half the state each time... which opens the door for somebody else to grab the lock when we are half changed!!

I think there should be one lock and one proxy state - which does not try to fully duplicate the individual connection states of the two connections. The proxy state should be something like: CONNECTING, OPEN, CLIENT_CLOSING, SERVER_CLOSING, PROXY_CLOSING, CLOSED

@sbordet what do you think? have I got something wrong and am pushing @lachlan-roberts opposite to your instructions?

gregw · 2019-02-20T05:35:42Z

jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java

+            @Override
+            public void succeeded()
+            {
+                callback1.succeeded();


what if callback1.succeeded() throws an unchecked exception?

@gregw then we need to review the whole Jetty codebase for such possibility.
@lachlan-roberts, I'm not keen having these 2 from() methods - no other code uses them apart websocket code.

@sbordet the from style is something we've been using in websockets. They are not used elsewhere because most of the code was written pre-lambda. While we might perhaps have too many from methods now, I do think we should at least consider if they are utility and are usable elsewhere.

the (Callback, Throwable) version is used 5 places in WebSocketProxy and also 2 places in WebSocketChannel.
the (Callback, Callback) one is used 4 times and only in WebSocketProxy

gregw · 2019-02-20T05:36:14Z

jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java

+            @Override
+            public void failed(Throwable x)
+            {
+                callback1.failed(x);


what if callback1 throws an unchecked exception?

gregw · 2019-02-20T05:38:38Z

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

@@ -66,10 +66,14 @@
    /**
     * Async notification that Connection is being opened.
     * <p>
-     * FrameHandler can write during this call, but will not receive frames until
-     * the onOpen() completes.
+     * FrameHandler can write during this call, but can not receive frames until the callback is succeeded.


not exactly correct.... it cannot receive frames until demand(int) has been called with demand >0. This will be done automatically for non demandable FrameHandlers when the callback is succeeded. For Demandable handlers, it is an ISE to call demand prior to succeeding the onOpen callback

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

gregw · 2019-02-20T05:41:28Z

...t/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/BasicFrameHandler.java

+    public void onOpen(CoreSession coreSession, Callback callback)
+    {
+        session = coreSession;
+        System.err.println(name + " onOpen(): " + session);


perhaps printlns should be converted to debugs

gregw · 2019-02-20T05:44:59Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+        @Override
+        public void onOpen(CoreSession session, Callback callback)
+        {
+            System.err.println(toString() + " onOpen(): " + session);


println to debug

gregw · 2019-02-20T05:48:59Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+                        break;
+
+                    case FAILED:
+                        failure = error;


should add t as a suppressed exception to failure?

gregw · 2019-02-20T05:55:28Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+        NOT_OPEN,
+        CONNECTING,
+        OPEN,
+        ICLOSED,


is ICLOSED really the same state for receiving a CLOSE from either client2proxy or proxy2server?
It feels like we are forgetting vital info that could be useful for handling a subsequent error/timeout ?

I also don't like the name ICLOSED, at the very least it should be CLIENT2PROXY_ICLOSED or CLIENT2PROXY_CLOSING

ISHUT and OSHUT.

@sbordet I'm not so sure. Why are we duplicating connection state in the proxy layer? I think we just need a single proxy state that captures what the proxy is currently doing and not what the state of it's connections are.

gregw · 2019-02-20T06:03:15Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+                        break;
+
+                    case ICLOSED:
+                        if (frame.getOpCode() == OpCode.CLOSE)


I think this is wrong? When client2proxy receives the close frame it's onFrame method will change to ICLOSED and then send the close from using this method, so this will immediately go to CLOSED.

I also don't like the double state change of grabbing lock and doing state handling in onFrame and then doing the same again in send. I think we should be able to grab lock once, do the state change, do the action and then any subsequent state change needed should be done in the callback. send(...) is only called from onFrame so all the state change logic can be done there.

gregw · 2019-02-20T06:07:56Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+            Throwable failure = null;
+            synchronized (lock)
+            {
+                switch (state)


likewise, connect is only ever called from client2proxy.onOpen so why does it need to grab the lock and check/change state again?

Oh I see... there are two states......

sbordet · 2019-02-20T14:57:51Z

@gregw, this is a different approach based on states that I wanted @lachlan-roberts to code to figure out whether he liked it better, whether it was clearer to read, etc.
I preferred to have him code with more states than necessary, but I don't think the current implementation has particular problems.
Remember that it has to be fully duplex, so if the proxy receives a close frame, then that side is half closed, but in the meantime it should still be able to receive messages from the server and forward them.

Having said that, do you see actual problems with the current implementation?

I ask because your comments seems more on the line of "I would have done it differently" rather than "there is an actual problem if this and that happen".
I'll review it again with @lachlan-roberts.

The initial goal of this work, i.e. to have a proof that the core APIs allow to write a fully async proxy, seems satisfied to me.
So it may not be the prettiest proxy, but it shows that the API are correct and it was a good exercise.

sbordet

@lachlan-roberts good job!

I am mostly complaining about the core APIs, which I think need some improvements in at least 2 places:

handling of concurrent writes
return type of the connect() method

I also think that onFrame() has too many semantics; I would have preferred that close frames were delivered to a different method, so that it would have been easier to write handlers (now they all require - all the times - if (frame.getType() == CLOSE), and the callback object does 2 different things depending on the frame type.

sbordet · 2019-02-20T17:25:39Z

jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java

+            @Override
+            public void succeeded()
+            {
+                callback1.succeeded();


@gregw then we need to review the whole Jetty codebase for such possibility.
@lachlan-roberts, I'm not keen having these 2 from() methods - no other code uses them apart websocket code.

sbordet · 2019-02-20T17:44:41Z

.../websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java

+        CompletableFuture<CoreSession> response = _client.connect(upgradeRequest);
+        response.get(5, TimeUnit.SECONDS);
+        clientHandler.sendText("hello world");
+        clientHandler.close("standard close");


You should not be able to write the 2 lines above - sendText() + close() - without a callback.
How do you avoid a tight loop sending frames?

for (int i = 0; i < 1000; ++i) { session.sendFrame(frame, Callback.NOOP, false); }

This should throw a WritePendingException, no?

You'd think! That was also a surprise to me! But we definitely have code that allows multiple concurrent writes like this and there is no write pending exception! That is partially why we have the write flusher above the extension stack to serialise such parallel calls. If they happen in a bad order, then eventually a callback will be failed and the connection closed. So it is only in very simple use-cases that a framework/app can actually have multiple threads calling in this way, but we do allow it.

I'm not wedded to doing it this way, but it was how it was.... @lachlan-roberts can you check if this is a semantic private to our API, or does the JSR also allow such parallel writes without a WritePendingException?

@gregw the JSR allows you to do parallel writes in the same way

@lachlan-roberts thanks! So yes I think we are stuck with the parallel call semantic. I don't think it costs us anything as we have the queue and flusher anyway.

sbordet · 2019-02-20T17:48:34Z

.../websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java

+        WebSocketProxy.Server2Proxy proxyServerSide = proxy.server2Proxy;
+
+        ClientUpgradeRequest upgradeRequest = ClientUpgradeRequest.from(_client, new URI("ws://localhost:8080/proxy/a"), clientHandler);
+        CompletableFuture<CoreSession> response = _client.connect(upgradeRequest);


Returning here a CompletableFuture is a wrong API because the Future API can be used (and it is in the tests).
I would rather pass a Promise<CoreSession> as parameter instead.

I thought we already had this discussion and decided that it was best to leave it this way with the CompletableFuture. Ultimately it really should just be the onOpen callback that is needed, but this is provided for backward compatibility and as a convenience.... even if it allows non async get() calls.

Happy to review this, but perhaps outside of the scope of this PR.

sbordet · 2019-02-20T17:56:40Z

.../websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java

+        try (StacklessLogging stacklessLogging = new StacklessLogging(WebSocketChannel.class))
+        {
+            CompletableFuture<CoreSession> response = _client.connect(clientHandler, new URI("ws://localhost:8080/proxy/"));
+            response.get(5, TimeUnit.SECONDS);


This is so wrong 😃
You throw an exception from clientHandler.onOpen(), but response.get() does not throw and actually returns you back the session.
I don't think we want this behavior because there is no difference with the case where you don't throw from onOpen().
We definitely need to discuss this.

as previously discussed this is a bit strange.... but if onOpen throws, then the connection is not opened and the future should not be succeeded... so get() should throw I thought? @lachlan-roberts ???

@gregw If onOpen throws it is immediately caught in WebSocketChannel and the callback is failed which will attempt a close. The CompletableFuture will still be succeeded and return the CoreSession (but the session should already be closed by the callback).

@lachlan-roberts is there a way that we can fail the CompletableFuture if anything goes wrong up until the call to onOpen returns? I'm not a huge fan of the CompletableFuture, but I'm OK to let it remain as a backward compatible convenience... but it has to work reasonably and I think failing of failure is reasonable.

sbordet · 2019-02-20T18:02:27Z

.../websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java

+        frame = proxyClientSide.receivedFrames.poll();
+        closeStatus = CloseStatus.getCloseStatus(frame);
+        assertThat(closeStatus.getCode(), is(CloseStatus.SERVER_ERROR));
+        assertThat(closeStatus.getReason(), is("intentionally throwing in server onFrame()"));


This is weird. proxyClientSide.receivedFrames contains the frames that the client sent to the proxy. Why does it have a close frame with SERVER_ERROR?
I would have expected that to be in proxyServerSide.receivedFrames.
If this is the reply to the close frame sent by the server, then you should reorganize the test to do the assertions in order, with a bit of comment on what should happen.

I'm guessing this is because there is no good client side equivalent to SERVER_ERROR. ie there is no CLIENT_ERROR.

sbordet · 2019-02-20T18:04:15Z

.../websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxyTest.java

+    @Test
+    public void testServerErrorClientNoResponse() throws Exception
+    {
+        serverFrameHandler.throwOnFrame();


I think this should be serverFrameHandler.noResponseOnFrame()?

this test was to test if what would happen in the Proxy if the server had an error and the client didn't respond to the close frame it was sent, so the client handler is overridden to not complete the close frame callback

sbordet · 2019-02-20T18:06:25Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+        NOT_OPEN,
+        CONNECTING,
+        OPEN,
+        ICLOSED,


ISHUT and OSHUT.

sbordet · 2019-02-20T18:09:43Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+            if (failServer2Proxy)
+                server2Proxy.fail(failure, callback);
+            else if (failure != null)
+                callback.failed(failure);


I think that if you end up in the default case, you really want to fail the serverToProxy as well.
If that's the case, the 2 failure cases become one and the code simplifies.

sbordet · 2019-02-20T18:26:15Z

...cket/websocket-core/src/test/java/org/eclipse/jetty/websocket/core/proxy/WebSocketProxy.java

+                        {
+                            state = State.ICLOSED;
+                            closeCallback = callback;
+                            sendCallback = Callback.from(()->{}, callback::failed);


You should add a comment about why you store closeCallback here - in 6 months from now you won't remember.
Note how the fact that you need to store the callback is because you know what the callback is doing, and you want to delay that action.
This means that the javadocs for onFrame() should explain in great details what the callback is doing depending on the various frame types.
This is not ideal: you want different methods to have different semantics, not the same method having different semantics based on the frame type.

gregw · 2019-02-21T00:41:42Z

@sbordet I'm on board with the different approach of using states and synchronise blocks. I was already concerned that having two atomic states was almost certainly wrong... just we din't know how yet. However, my concerns are more than just "I would have done it differently". I am concerned that the approach of grabbing the lock in onFrame and then again in send needlessly allows for other actions to intervene between the two events.

It should be sufficient to grab the lock once in onFrame, determine the action to be taken, change state, let the lock go and then attempt the action. All subsequent state changes should then be attempted in the callback success or failure of the action (of course grabbing the lock again). Having two states just needlessly increases the combinations and complexity of the state machine. Yes of courses it has to be full duplex and closure can come from both sides (or even spontaneously in the proxy), but I still don't think that needs dual states.

Said another way, the send/connect methods are only ever called as a result of onFrame/onOpen, so there is no need to grab locks again and change state. Either the send/connect succeeds or it doesn't - eitherway the callback should handle that. Allowing the possibility that we wanted to do a send/connect but some other event happened first doesn't add anything other than the need to duplicate logic already provided by core.

gregw · 2019-02-21T00:54:37Z

@sbordet with regards to your comment about onFrame needing to be split into onClose etc.
The problem with this is that different frameworks built on core will want the split differently. Some will control vs data frame, some will want just close vs non-close while others will split out into onText, onContinuation, onBinary, onPing etc. Furthermore, some also have the need to intercept all frames regardless of type. Thus any split out that core does will not be right for some frameworks and will require duplicate handling for any onFrame interception that they wish to provide. At best we will be switching on frame type twice, at worst we can be duplicating logic which invites programming errors.

Core is rarely intended to be used directly like this, so keeping it simple and not wrong is more important than a little bit of convenience.

Signed-off-by: lachan-roberts <lachlan@webtide.com>

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

gregw · 2019-03-05T22:22:56Z

@lachlan-roberts are there any non proxy changes in this PR that should be merged prior to resolving the design of the proxy itself?

lachlan-roberts · 2019-03-05T22:34:22Z

The changes to WebSocketChannel, Callback and FrameHandler could be considered separately to the proxy. There are some bug fixes in the changes to WebSocketChannel, utility methods in Callback and just javadoc changes to FrameHandler.

Should I create a separate PR for these changes?

gregw · 2019-03-06T05:04:59Z

yes

…-3170-websocket-proxy Signed-off-by: lachan-roberts <lachlan@webtide.com>

…-3170-websocket-proxy Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

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

lachlan-roberts · 2019-06-12T02:18:22Z

@gregw @sbordet I have updated this PR and it is ready to be reviewed again

gregw · 2019-06-18T07:38:00Z

This is not perfect... but better in than out so it will be tested and maintained. So I think we should merge. @sbordet if you agree then accept changes?

lachlan-roberts added 7 commits February 19, 2019 09:29

Issue jetty#3170 - WebSocket Proxy PoC

c1e3e57

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

Issue jetty#3170 - WebSocket Proxy handle error and failure conditions

55f82ce

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

Issue jetty#3170 - stop WebSocketCoreClient on close or failure

bc3df4f

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

Issue jetty#3170 - WebSocketProxy separate actions from the state cha…

001bc8f

…nges only do state changes inside synchronized blocks, remember what action to do and only do this outside of the synchronized block Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts force-pushed the jetty-10.0.x-3170-websocket-proxy branch 2 times, most recently from 3eccc10 to d6129f5 Compare February 18, 2019 22:35

Issue jetty#3170 - WebSocketProxy cleanup

3e33b35

removing old versions of the proxy frame handler adding licence headers Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts force-pushed the jetty-10.0.x-3170-websocket-proxy branch from d6129f5 to 3e33b35 Compare February 18, 2019 22:38

lachlan-roberts marked this pull request as ready for review February 18, 2019 22:53

gregw requested changes Feb 20, 2019

View reviewed changes

sbordet requested changes Feb 20, 2019

View reviewed changes

Issue jetty#3170 - changes from review

bd50e29

Signed-off-by: lachan-roberts <lachlan@webtide.com>

Issue jetty#3170 - fail the CompletableFuture if onOpen throws

37ab716

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

lachlan-roberts force-pushed the jetty-10.0.x-3170-websocket-proxy branch from 325c25b to 37ab716 Compare February 28, 2019 07:13

lachlan-roberts mentioned this pull request Mar 6, 2019

WebSocketChannel bug fixes, cleanup and javadoc updates for FrameHandler #3432

Merged

lachlan-roberts added 3 commits March 11, 2019 16:41

Merge remote-tracking branch 'eclipse/jetty-10.0.x' into jetty-10.0.x…

459c5ad

…-3170-websocket-proxy Signed-off-by: lachan-roberts <lachlan@webtide.com>

Merge remote-tracking branch 'eclipse/jetty-10.0.x' into jetty-10.0.x…

327783e

…-3170-websocket-proxy Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue jetty#3170 - cleanups of WebSocketProxy and WebSocketProxyTest

e007634

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

lachlan-roberts requested review from gregw and sbordet and removed request for gregw June 11, 2019 07:17

gregw approved these changes Jun 18, 2019

View reviewed changes

lachlan-roberts merged commit fa4abfa into jetty:jetty-10.0.x Jul 1, 2019

lachlan-roberts deleted the jetty-10.0.x-3170-websocket-proxy branch July 3, 2019 04:55

Issue #3170 - WebSocket Proxy #3365

Issue #3170 - WebSocket Proxy #3365

Conversation

lachlan-roberts commented Feb 15, 2019 • edited Loading

sbordet commented Feb 15, 2019

lachlan-roberts commented Feb 15, 2019

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet commented Feb 20, 2019

sbordet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachlan-roberts Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Feb 21, 2019

gregw commented Feb 21, 2019

gregw commented Mar 5, 2019

lachlan-roberts commented Mar 5, 2019

gregw commented Mar 6, 2019

lachlan-roberts commented Jun 12, 2019

gregw commented Jun 18, 2019

lachlan-roberts commented Feb 15, 2019 •

edited

Loading

sbordet Feb 20, 2019 •

edited

Loading

sbordet Feb 20, 2019 •

edited

Loading

lachlan-roberts Feb 22, 2019 •

edited

Loading