Add logic back to try to reconnect to the app websocket if socket is closed #279

matthme · 2024-10-14T21:45:34Z

Addresses #271

…closed

jost-s

Minor changes, please. LGTM, thanks!

src/api/app/websocket.ts

src/api/client.ts

test/e2e/common.ts

ThetaSinner · 2024-10-15T09:24:39Z

src/api/client.ts

+        // typescript forgets in this promise scope that `this.url` is not undefined
+        const socket = new IsoWebSocket(this.url as URL, this.options);
+        this.socket = socket;
+        socket.onerror = (errorEvent) => {


Can this tell the difference between an invalid token and other kinds of error?

It seems like the user experience wouldn't be great if requests always result in a reconnect that fails when a temporary token was in use. If we can tell the token is invalid here, it would be good to clear it so that the client stops trying to reconnect

With an invalid token what happens is that you get the "Client disconnected with pending requests" error and this onError callback here is never called in the first place.

I don't actually understand where the "client disconnected with pending requests" occurs...but if I log something in that onError callback it's not getting logged when it fails due to an invalid token.

Interesting. Well Holochain won't send back anything to tell the client that the connection is going to be closed, it'll just close the connection from its end. Where is that error thrown, can/should it be caught?

I confirm that his happens. It comes in as a close event on the websocket with code 1005 which means "No status received". It's indistinguishable from another unexpected close event which usually is 1005 or 1006. For this reconnect we could register another close handler that unsets the token, but it's not 100 % certain that the close was due to an invalid token.

There's a specific InvalidTokenError now which only happens on failed automatic websocket reconnect. If the reconnect fails, the stored token is deleted. No risk of this happening in a loop, because the reconnects are only attempted as part of a request being made by the client consumer.

test/e2e/index.ts

ThetaSinner · 2024-10-15T09:30:44Z

test/e2e/common.ts

@@ -97,7 +97,8 @@ export const withConductor =
  };

 export const installAppAndDna = async (
-  adminPort: number
+  adminPort: number,
+  nonExpiringToken?: boolean


Would it make sense to call this parameter singleUseToken to stay close to the conductor API naming?

And then have it be true by default? We can do that yes. It's single-use and expiring...I could also make two parameters for expiry and single use.

I changed it to take two separate parameters now to be aligned with the conductor API naming: d6a7db7

That works yes, just expose the API with defaults. I was thinking that single use could have a timeout and when it's false then it could have no expiry. I wouldn't be opposed to the client exposing the API that way because those are the two expected configurations really

Though it's probably useful to let people just have access to these

ThetaSinner · 2024-10-15T09:34:20Z

test/e2e/index.ts

+);
+
+test(
+  "client fails to reconnect to websocket if closed before making a zome call and does not end up in a loop",


This isn't quite testing what I was concerned about with looping. It's repeatedly reconnecting with an invalid token that I was concerned about. If the caller isn't careful about which error is which then they might treat this error as retryable and keep trying to reconnect. I'd prefer if this test could make two zome calls and the second one got a different error message back asking the caller to provide new credentials

Well, configuring the message to ask for new credentials doesn't work I think due to the "client closed with pending requests" error that doesn't even let us define the error message here.

Maybe I can look into handling this close event better in the first place.

But I'm not sure whether we should conflate this with this PR

There is no way of modifying the error in a way to ask the user to provide a valid token. Any zome call with an invalid token will now lead to the following error:

ClientClosedWithPendingRequests: client closed with pending requests - close event code: 1005, request id: 1

And this seems to be a result of how the conductor handles things and cannot be affected at the js side as I understand it. This error however is distinguishable by a caller from a "normal" zome call error if required so I don't think there's more we can do here without touching the conductor behavior.

jost-s · 2024-10-17T20:16:15Z

src/api/client.ts

@@ -142,18 +63,22 @@ export class WsClient extends Emittery {
  static connect(url: URL, options?: WsClientOptions) {
    return new Promise<WsClient>((resolve, reject) => {
      const socket = new IsoWebSocket(url, options);
-      socket.onerror = (errorEvent) => {
+      socket.addEventListener("error", (errorEvent) => {


addEventListener used throughout now, with one-off listeners for all calls returning a promise.

jost-s · 2024-10-17T20:18:13Z

src/api/client.ts

-      // Message just needs to be sent first, no need to wait for a response or even require a flush
-      resolve(null);
+      // Wait before resolving in case authentication fails.
+      setTimeout(() => {


If authentication fails, it'll still return a resolve. Waiting here prevents that. It's not a universal fool-proof way, ideally a ping can be sent after authenticating or Holochain returns an error or closes the websocket with a specific error code.

Great stuff, thanks for polishing this 👍 . This idea did not occur to me.

jost-s · 2024-10-17T20:18:53Z

src/api/client.ts

@@ -233,6 +200,144 @@ export class WsClient extends Emittery {
    this.index += 1;
  }

+  private registerMessageListener(socket: IsoWebSocket) {


Unchanged, just separated into a function and moved within the file.

jost-s · 2024-10-17T20:20:36Z

src/api/client.ts

+      this.socket.addEventListener(
+        "open",
+        async (_) => {
+          const encodedMsg = encode({


I would have loved to call authenticate again here, but that's so convoluted with the sendHandler parameter and the requesterTransformer and _request and promiseTimeout, it's head-wrecking. Would be brilliant to simplify that part of the client.

jost-s · 2024-10-17T20:21:08Z

test/e2e/index.ts

@@ -422,6 +422,28 @@ test(
  })
 );

+test(


Test unhappy websocket authentication path.

jost-s · 2024-10-17T20:25:55Z

test/e2e/index.ts

+        "error should be of type HolochainError"
+      );
+      assert(error instanceof HolochainError);
+      t.equal(error.name, "InvalidTokenError", "expected an InvalidTokenError");


For the future - create an enum with all possible error kinds #281

jost-s · 2024-10-18T03:12:38Z

src/api/common.ts

@@ -96,6 +96,7 @@ export const promiseTimeout = (
        return res(a);
      })
      .catch((e) => {
+        clearTimeout(id);


Without clearing the timeout, failed requests were waiting for the duration of the timeout (60 seconds) before returning.

ThetaSinner · 2024-10-28T17:35:43Z

src/api/app/websocket.ts

@@ -77,6 +82,7 @@ export class AppWebsocket implements AppClient {
    CallZomeResponseGeneric<Uint8Array>,
    CallZomeResponse
  >;
+  private readonly appAuthenticationToken: AppAuthenticationToken;


I don't believe this is very high risk, but I don't feel comfortable not pointing this out. Storing this token may expose it to an injected script. If it is exfiltrated then it is re-usable elsewhere.

I much prefer the single-use tokens but I understand that this is being done for UX.

It is, and it's similar to the zome call signing credentials. They're equally stored in JavaScript memory for convenience, which compromises security to a degree.

ThetaSinner · 2024-10-28T17:37:10Z

src/api/client.ts

+      const invalidTokenCloseListener = (
+        closeEvent: IsoWebSocket.CloseEvent
+      ) => {
+        reject(


If we're rejecting because of an invalid token, should this.authenticationToken not be cleared here?

Yes, absolutely! d3d8412

ThetaSinner · 2024-10-28T17:39:00Z

test/e2e/index.ts

+      t.assert(error instanceof HolochainError);
+      assert(error instanceof HolochainError);
+      t.equal(error.name, "InvalidTokenError", "expected InvalidTokenError");
+    }


If at this point you call it again, I'd expect to get a "token is required", rather than "invalid token error"?

This only calls once. What are you referring to by "you call it again"?

I see now this is the wrong test to ask this question. I'm looking for a test where we

connect with a valid token that is single use,

lose the connection then try to make a zome call, but now have an invalid token,

so we get a invalid token error

then try to make another zome call and we should get a "no token" error or similar because the reconnect can't be attempted

Is that what the test below with the comment "Websocket reconnection has failed and subsequent calls should just return" is doing?

Ah, yes, exactly. The subsequent call is attempted after not handling the invalid token error. In a real app, the invalid token error must be handled and a new token be requested before attempting another call.

Sweet, okay. I'm caught up now and my original concern with this change has been addressed. Thank you :)

* backport #279 * updated changelog * workflow back to main branch

add logic back to try to reconnect to the app websocket if socket is …

2427bbd

…closed

matthme requested review from ThetaSinner and jost-s October 14, 2024 21:45

jost-s approved these changes Oct 15, 2024

View reviewed changes

src/api/app/websocket.ts Outdated Show resolved Hide resolved

src/api/client.ts Outdated Show resolved Hide resolved

test/e2e/common.ts Outdated Show resolved Hide resolved

cleanup

46d887e

ThetaSinner reviewed Oct 15, 2024

View reviewed changes

test/e2e/index.ts Outdated Show resolved Hide resolved

ThetaSinner reviewed Oct 15, 2024

View reviewed changes

test/e2e/index.ts Show resolved Hide resolved

ThetaSinner reviewed Oct 15, 2024

View reviewed changes

include error in test failure

76eb323

ThetaSinner reviewed Oct 15, 2024

View reviewed changes

matthme and others added 7 commits October 15, 2024 12:05

make arguments more aligned with conductor API

d6a7db7

fix ws options type

1ff4ba2

fix test

d95a4d1

updated test to assert on error message

6dcaeec

refactor(app): remove unused fn containsCell

939c11c

refactor(client): unify usage of listeners & handle invalid app tokens

868e5d1

docs: update changelog

b14870c

jost-s requested a review from ThetaSinner October 17, 2024 20:11

revert: make WsClientOptions optional

b38aa5b

jost-s reviewed Oct 17, 2024

View reviewed changes

test/e2e/index.ts

@@ -422,6 +422,28 @@ test(

})

);

test(

Copy link

Contributor

jost-s Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test unhappy websocket authentication path.

jost-s reviewed Oct 17, 2024

View reviewed changes

jost-s added 2 commits October 17, 2024 19:49

fix(common): return request error without awaiting timeout

382405b

Merge branch 'main' into fix/try-reconnect-if-closed

efe02c5

jost-s reviewed Oct 18, 2024

View reviewed changes

ThetaSinner reviewed Oct 28, 2024

View reviewed changes

Jost Schulte added 2 commits October 28, 2024 13:14

fix(client): unset auth token on rejected connection attempt

d3d8412

build(nix): update flake

3ffb508

ThetaSinner approved these changes Oct 28, 2024

View reviewed changes

jost-s enabled auto-merge (squash) October 28, 2024 19:31

jost-s merged commit d6b8893 into main Oct 28, 2024
11 checks passed

jost-s deleted the fix/try-reconnect-if-closed branch October 28, 2024 19:35

matthme added a commit that referenced this pull request Oct 29, 2024

backport #279

bc91367

jost-s pushed a commit that referenced this pull request Oct 29, 2024

feat(client): try to reconnect to app websocket if closed (#283)

81effb8

* backport #279 * updated changelog * workflow back to main branch

@@ @@ -422,6 +422,28 @@ test( @@
                 })
               );
+              test(

Add logic back to try to reconnect to the app websocket if socket is closed #279

Add logic back to try to reconnect to the app websocket if socket is closed #279

Conversation

matthme commented Oct 14, 2024

jost-s 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

matthme Oct 15, 2024 • 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

matthme Oct 15, 2024 • 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

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

jost-s Oct 28, 2024 • 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

matthme Oct 15, 2024 •

edited

Loading

matthme Oct 15, 2024 •

edited

Loading

jost-s Oct 28, 2024 •

edited

Loading