JS API should attempt to reconnect to the gRPC server #3502

niloc132 · 2023-03-07T22:37:49Z

As long as a user's session on the server remains active, this should enable a client to reconnect to that session, and continue using already-exported objects. Some of the work is restored here for #3501 to be finished as well, but the client would need a way to create a new session without user interaction.

The dh.IdeConnection.HACK_CONNECTION_FAILURE event is now deprecated, and should be removed. Clients should instead use the EVENT_DISCONNECTED and EVENT_RECONNECTED events to signal to the user that an object isn't available.

Also fixes a bug where the last seen log item is replayed incorrectly.

Fixes #730
Fixes #225

...client-api/src/main/java/io/deephaven/web/client/api/grpc/MultiplexedWebsocketTransport.java

niloc132 · 2023-03-08T03:12:01Z

web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java

                        }
                        return;
                    }
                    assert success != null;

                    // welp; the server is gone -- let everyone know
+                    connectionLost();
+
                    info.notifyConnectionError(new ResponseStreamWrapper.Status() {


Removing the notifyConnectionError here, and instead going with a disconnect event exposes a missing case that we need to consider for DHC, how to signal that the server has shut down and told the client this, but how to potentially handle the server coming back again afterwards. We probably need a new event, or metadata on the disconnect event, but likewise should probably welcome the server back by prompting another login (right now we'll get the "reauth failed" event) by the user.

Subscribers of TerminationNotification get a tombstone from the server with abnormalTermination set to false. We can probably use this to indicate a normal / expected shutdown? Though, I personally get these on my machines as the DH instances restart whenever they are redeployed. Either way, maybe this is the path you want to use?

We do get "expected shutdown", but if you are testing an application or some server side debugging, you're going to edit, restart, edit, restart - an expected shutdown isn't the same as "never come back without user interaction".

Success or failure we can tell the client that the disconnect happened, but the question then is do we say "the server is down (here's the error|this was on purpose)" or just say "its down, it might be up again soon, please dismiss the message"?

Added a new event on IdeConnection, EVENT_SHUTDOWN that will emit just the status message from the shutdown, rather than the full grpc status. It will only fire if the client gets notification that the server is explicitly going away, as opposed to losing that stream (or any other stream).

nbauernfeind · 2023-03-10T00:05:01Z

server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java

@@ -366,7 +366,7 @@ public void record(LogBufferRecord record) {
            }
            // since the subscribe() method auto-replays all existing logs, filter to just once this consumer probably
            // hasn't seen
-            if (record.getTimestampMicros() < request.getLastSeenLogTimestamp()) {
+            if (record.getTimestampMicros() <= request.getLastSeenLogTimestamp()) {


This is just a note, not really feedback or a request other than to consider whether or not we should change the API.

Interesting. The listener could miss log messages that occurred at the same timestamp, but I see how the alternative guarantees at least one duplicate message. Maybe a better API would count the log messages and you just provide an offset. (This is probably why Kafka uses an offset instead of timestamp for new subscriptions.)

Ah that's fair. Dup messages were confusing me, and the comment made it appear that this was "filtering to [ones] this consumer probably hadn't seen" meant something other than "will definitely always dup the last message seen, and maybe more". @devinrsmith any thoughts? I think you touched this last?

I know we will sometimes have gaps since this is implemented as a ring buffer, and maybe we tacitly accept that?

The "guarantees" around the log buffer API are sketchy at best - the consumer never knows if they actually missed log messages or not. I'm happy to "break" the current API w/ the change as Colin has here... all else equal, if the client wants to they can request timestamp-1, and potentially handle dubious de-duping logic if necessary themselves.

Okay, so I'll revert this and fix the comment instead?

web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java

nbauernfeind · 2023-03-10T00:49:17Z

web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java

                        }
                        return;
                    }
                    assert success != null;

                    // welp; the server is gone -- let everyone know
+                    connectionLost();
+
                    info.notifyConnectionError(new ResponseStreamWrapper.Status() {


Subscribers of TerminationNotification get a tombstone from the server with abnormalTermination set to false. We can probably use this to indicate a normal / expected shutdown? Though, I personally get these on my machines as the DH instances restart whenever they are redeployed. Either way, maybe this is the path you want to use?

devinrsmith

I'm a bit out of my depth when it comes to reviewing JS API code.

Is there an sample client code to show EVENT_DISCONNECTED / EVENT_RECONNECTED event handling? Is there any "easy" way to simulate server disconnections?

devinrsmith · 2023-03-10T17:50:23Z

web/client-api/src/main/java/io/deephaven/web/client/ide/IdeConnection.java

+    public static final String EVENT_DISCONNECT = "disconnect";
+    public static final String EVENT_RECONNECT = "reconnect";


I don't see these being used - are these meant to be referenced by client code?

Yes, we declare constants for events that can be fired on the class that can fire them.

Yes, we decorate the event names on the JS types, so we can easily document what events are emitted from a type. We could probably do better with types (esp with incoming generated TS types), but for now this is how we do it.

(sorry for the dup, i had this open and it didnt show the old message, so thought i hadnt responded...)

mofojed · 2023-03-14T16:00:02Z

I opened a draft PR for UI to update for this: deephaven/web-client-ui#1149
I'm not sure how best to test this, but I did just try with just killing the connection to see what happens. Couple notes:

No error details are passed with the EVENT_DISCONNECT. Would be useful if there are possible different disconnect scenarios (such as "socket going down and we might come back" vs. "worker shutdown")
The disconnect event seems to be getting fired 3 times:

niloc132 · 2023-03-16T14:12:53Z

No error details are passed with the EVENT_DISCONNECT. Would be useful if there are possible different disconnect scenarios (such as "socket going down and we might come back" vs. "worker shutdown")

"worker shutdown" is definitely a "we might come back" scenario? At the very least, DnD PQs will have to anticipate this, and will expect (one day) to be able to attempt reconnect here (though at present the JS API can't support this, only due to re-auth concerns).

The disconnect event seems to be getting fired 3 times:...

This is consistent with dhe workers, both PQs and ad hoc, right? Its actually N times, based on how long you are disconnected for, how fast the reconnect retries to connect again. The original idea was that the UI could get a sense of how long it had been since the last try.

On the other hand, Tables (and formerly treetables, need to look closer at DHC here) only fire once, and suppress all later events until reconnected.

mofojed

Looks good with my Web UI changes: deephaven/web-client-ui#1149

niloc132 · 2023-03-23T19:06:27Z

I added one more commit to remove a logged error message that can occur when the user's session is resumed via a new connection.

niloc132 added 12 commits February 16, 2023 10:11

First draft of reconnect by reusing the existing session

97927d8

wip on complete reconnect

1278c62

Merge branch 'main' into 730-js-api-reconnect

2103915

first working draft of same-session reconnect

f7c5318

Merge remote-tracking branch 'upstream/main' into 730-js-api-reconnect

d8aded5

Rewire logs

fc4e156

Tidy up for review

8c79f16

import cleanup

48d9647

Merge remote-tracking branch 'upstream/main' into 730-js-api-reconnect

e25096d

self-review

081ddee

remove unneeded TODO

bf81814

Warn use of old event

dc4a81c

niloc132 added jsapi NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Mar 7, 2023

niloc132 added this to the Mar 2023 milestone Mar 7, 2023

niloc132 requested review from nbauernfeind, vbabich, mofojed and devinrsmith March 7, 2023 22:37

niloc132 mentioned this pull request Mar 7, 2023

Support reconnecting to the server deephaven/web-client-ui#1140

Closed

niloc132 commented Mar 8, 2023

View reviewed changes

nbauernfeind reviewed Mar 10, 2023

View reviewed changes

devinrsmith reviewed Mar 10, 2023

View reviewed changes

niloc132 added 4 commits March 16, 2023 14:30

Merge remote-tracking branch 'upstream/main' into 730-js-api-reconnect

4db8a1a

Add a shutdown event

2b9ffb8

Null out transport when done to ensure it can't be released twice

7f7e268

This is getting called multiple times, check for null

2590e80

mofojed mentioned this pull request Mar 23, 2023

feat: JS API reconnect deephaven/web-client-ui#1149

Merged

mofojed previously approved these changes Mar 23, 2023

View reviewed changes

Reduce logging of an error that happens on disconnect/reconnect

b173bd3

niloc132 dismissed mofojed’s stale review via b173bd3 March 23, 2023 18:51

mofojed previously approved these changes Mar 23, 2023

View reviewed changes

Update proto docs

8dfc31f

niloc132 dismissed mofojed’s stale review via 8dfc31f March 23, 2023 21:25

mofojed approved these changes Mar 24, 2023

View reviewed changes

niloc132 merged commit 9a5501b into deephaven:main Mar 24, 2023

github-actions bot locked and limited conversation to collaborators Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS API should attempt to reconnect to the gRPC server #3502

JS API should attempt to reconnect to the gRPC server #3502

niloc132 commented Mar 7, 2023

niloc132 Mar 8, 2023

nbauernfeind Mar 10, 2023

niloc132 Mar 13, 2023

niloc132 Mar 17, 2023

nbauernfeind Mar 10, 2023

niloc132 Mar 16, 2023

devinrsmith Mar 16, 2023

niloc132 Mar 17, 2023

nbauernfeind Mar 10, 2023

devinrsmith left a comment

devinrsmith Mar 10, 2023

niloc132 Mar 14, 2023

niloc132 Mar 16, 2023

niloc132 Mar 16, 2023

mofojed commented Mar 14, 2023

niloc132 commented Mar 16, 2023

mofojed left a comment

niloc132 commented Mar 23, 2023

		public static final String EVENT_DISCONNECT = "disconnect";
		public static final String EVENT_RECONNECT = "reconnect";

JS API should attempt to reconnect to the gRPC server #3502

JS API should attempt to reconnect to the gRPC server #3502

Conversation

niloc132 commented Mar 7, 2023

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

devinrsmith 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

mofojed commented Mar 14, 2023

niloc132 commented Mar 16, 2023

mofojed left a comment

Choose a reason for hiding this comment

niloc132 commented Mar 23, 2023