MessagingSession reconnects with refreshed endpoint and credentials if needed #2400

dpwspoon · 2022-08-15T22:16:47Z

Issue #:

Fixs 4401 from #2372

Description of changes:

MessagingSession reconnects with refreshed endpoint and credentials if needed. This is an updated version of #2180 that was reverted

Testing:

Tested with demo app (see below)

Can these tested using a demo application? Please provide reproducible step-by-step instructions.

Run the messagingSession demo here: https://github.com/aws/amazon-chime-sdk-js/tree/main/demos/browser. npm run start --app=messagingSession
connect with an app instance user
Disconnect Wifi
wait ~20 secs
Reconnect Wifi and see reconnect

Then test again with the following diff to see working on v2 version of client:

diff --git a/demos/browser/app/messagingSession/messagingSession.ts b/demos/browser/app/messagingSession/messagingSession.ts
index 2757af4d..9f408593 100644
--- a/demos/browser/app/messagingSession/messagingSession.ts
+++ b/demos/browser/app/messagingSession/messagingSession.ts
@@ -20,7 +20,7 @@ import {
   Versioning,
 } from 'amazon-chime-sdk-js';
 
-import { ChimeSDKMessagingClient } from '@aws-sdk/client-chime-sdk-messaging';
+import { ChimeSDKMessaging } from '@aws-sdk/client-chime-sdk-messaging';
 
 export class DemoMessagingSessionApp implements MessagingSessionObserver {
   static readonly BASE_URL: string = [
@@ -56,7 +56,7 @@ export class DemoMessagingSessionApp implements MessagingSessionObserver {
     document.getElementById('connect').addEventListener('click', async () => {
       try {
         const response = await this.fetchCredentials();
-        const chime = new ChimeSDKMessagingClient({ region: 'us-east-1', credentials: response });
+        const chime = new ChimeSDKMessaging({ region: 'us-east-1', credentials: response });
         this.userArn = (document.getElementById('userArn') as HTMLInputElement).value;
         this.sessionId = (document.getElementById('sessionId') as HTMLInputElement).value;
         this.prefetchSortByUnread = (document.getElementById('prefetchSortByUnread') as HTMLInputElement).checked;

During the testing suggest looking at networking console to see disconnect, retries, and retry success when wifi comes back up

Checklist:

Have you successfully run npm run build:release locally?

yes

Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?

yes these have been reviewed

Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?

no

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…f needed

duh17 · 2022-08-15T22:42:58Z

src/messagingsession/MessagingSessionConfiguration.ts

@@ -47,13 +47,17 @@ export default class MessagingSessionConfiguration {
  /**
   * Constructs a MessagingSessionConfiguration optionally with userArn, messaging session id, a messaging session
   * endpoint URL, and the chimeClient.
+   *
+   * endpointUrl is deprecated and should not be used. Internally it is resolved on connect via chimeClient if undefined, and


nit: would suggest mark it deprecated with @deprecated https://code.visualstudio.com/updates/v1_49#_deprecated-tag-support-for-javascript-and-typescript

src/messagingsession/DefaultMessagingSession.ts

devalevenkatesh · 2022-08-16T16:50:20Z

src/messagingsession/DefaultMessagingSession.ts

+        const closeEvent = new CloseEvent('close', {
+          wasClean: false,
+          code: 4999,
+          reason: 'Failed to getMessagingSessionEndpoint',


Since getMessagingSessionEndpoint is an function on AWS SDK v2, this would be irrelevant to AWS SDK v3. So should we change it to just 'Failed to get messaging session endpoint URL'?

thx will update

src/messagingsession/DefaultMessagingSession.ts

ltrung · 2022-08-16T17:08:19Z

src/messagingsession/DefaultMessagingSession.ts

@@ -105,7 +107,32 @@ export default class DefaultMessagingSession implements MessagingSession {
  }

  private async startConnecting(reconnecting: boolean): Promise<void> {
-    const signedUrl = await this.prepareWebSocketUrl();
+    // reconnect needs to re-resolve endpoint url, which will also refresh credentials on client if they are expired
+    let endpointUrl = !reconnecting ? this.configuration.endpointUrl : undefined;


nit: is this better to read?

let endpointUrl = this.configuration.endpointUrl; // reconnect needs to re-resolve endpoint url, which will also refresh credentials on client if they are expired if (reconnecting || endpointUrl === undefined) {

yeah, think so. Will update thx

package.json

ltrung · 2022-08-16T17:13:45Z

Can we verify with both v2 and v3 (different importing) to make sure it works ?

dpwspoon · 2022-08-16T20:06:22Z

Can we verify with both v2 and v3 (different importing) to make sure it works ?

Done, and updated directions on pr on how to do

src/messagingsession/DefaultMessagingSession.ts

devalevenkatesh · 2022-08-16T22:35:00Z

test/messagingsession/DefaultMessagingSession.test.ts

+          done();
+        },
+      });
+      messagingSession.start();


With v3, you have to use await as the messagingSession.start returns a promise now:
https://github.com/aws/amazon-chime-sdk-js/blob/main/guides/17_Migration_to_3_0.md#remove-aws-global-object-from-messagingsessionconfigurationts

Also could you please update the migration doc for the same and make the endpointUrl undefined there as well?

Also could you please update the migration doc for the same and make the endpointUrl undefined there as well?

Done

src/messagingsession/DefaultMessagingSession.ts

CHANGELOG.md

daviwith added 4 commits August 15, 2022 14:18

MessagingSession reconnects with refreshed endpoint and credentials i…

09463fe

…f needed

MessagingSession reconnect formatting

0f5816b

Logic for reconnect on failures fix

853dfad

Formatting fix in DefaultMessagingSession

ce994e2

dpwspoon requested a review from a team as a code owner August 15, 2022 22:16

duh17 previously approved these changes Aug 15, 2022

View reviewed changes

devalevenkatesh reviewed Aug 16, 2022

View reviewed changes

src/messagingsession/DefaultMessagingSession.ts Show resolved Hide resolved

devalevenkatesh reviewed Aug 16, 2022

View reviewed changes

src/messagingsession/DefaultMessagingSession.ts Show resolved Hide resolved

devalevenkatesh reviewed Aug 16, 2022

View reviewed changes

src/messagingsession/DefaultMessagingSession.ts Show resolved Hide resolved

ltrung reviewed Aug 16, 2022

View reviewed changes

devalevenkatesh reviewed Aug 16, 2022

View reviewed changes

package.json Show resolved Hide resolved

Minor clean up to DefaultMessagingSession reconnect

9c41a00

dpwspoon dismissed duh17’s stale review via 9c41a00 August 16, 2022 18:10

daviwith added 4 commits August 16, 2022 12:54

V2 default messaging session reconnect fix

f9d2dbc

Default Messaging Session fix no-explicit-any lint

d35d11d

Default Messaging Session fix no-explicit-any lint

aa9ef70

Default Messaging Session fix no-explicit-any lint

7b1d31c

ltrung reviewed Aug 16, 2022

View reviewed changes

src/messagingsession/DefaultMessagingSession.ts Outdated Show resolved Hide resolved

devalevenkatesh reviewed Aug 16, 2022

View reviewed changes

daviwith added 2 commits August 16, 2022 16:04

DefaultMessagingSession uses await on connect

2a9c9e8

DefaultMessagingSession uses await on connect

7ffd1ba

ltrung previously approved these changes Aug 17, 2022

View reviewed changes

devalevenkatesh reviewed Aug 17, 2022

View reviewed changes

src/messagingsession/DefaultMessagingSession.ts Show resolved Hide resolved

devalevenkatesh reviewed Aug 17, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

devalevenkatesh previously approved these changes Aug 17, 2022

View reviewed changes

Minor fixs to MessagingSession reconnect

00b4824

dpwspoon dismissed devalevenkatesh’s stale review via 00b4824 August 17, 2022 19:28

dpwspoon dismissed ltrung’s stale review via 00b4824 August 17, 2022 19:28

devalevenkatesh approved these changes Aug 17, 2022

View reviewed changes

devalevenkatesh merged commit bce872c into main Aug 17, 2022

devalevenkatesh deleted the feature/reconnect branch August 17, 2022 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MessagingSession reconnects with refreshed endpoint and credentials if needed #2400

MessagingSession reconnects with refreshed endpoint and credentials if needed #2400

dpwspoon commented Aug 15, 2022 •

edited

Loading

duh17 Aug 15, 2022

devalevenkatesh Aug 16, 2022

dpwspoon Aug 16, 2022

ltrung Aug 16, 2022

dpwspoon Aug 16, 2022

ltrung commented Aug 16, 2022

dpwspoon commented Aug 16, 2022

devalevenkatesh Aug 16, 2022

dpwspoon Aug 16, 2022

MessagingSession reconnects with refreshed endpoint and credentials if needed #2400

MessagingSession reconnects with refreshed endpoint and credentials if needed #2400

Conversation

dpwspoon commented Aug 15, 2022 • edited Loading

duh17 Aug 15, 2022

Choose a reason for hiding this comment

devalevenkatesh Aug 16, 2022

Choose a reason for hiding this comment

dpwspoon Aug 16, 2022

Choose a reason for hiding this comment

ltrung Aug 16, 2022

Choose a reason for hiding this comment

dpwspoon Aug 16, 2022

Choose a reason for hiding this comment

ltrung commented Aug 16, 2022

dpwspoon commented Aug 16, 2022

devalevenkatesh Aug 16, 2022

Choose a reason for hiding this comment

dpwspoon Aug 16, 2022

Choose a reason for hiding this comment

dpwspoon commented Aug 15, 2022 •

edited

Loading