Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Messaging session bug fix #2431

Closed
wants to merge 10 commits into from
Closed

Messaging session bug fix #2431

wants to merge 10 commits into from

Conversation

manasisurve
Copy link
Contributor

@manasisurve manasisurve commented Sep 12, 2022

Issue #:
#2372

Description of changes:

  1. Fix to break infinite reconnect loop in messagingSession on timeout. Reported here Chime chat(messaging) messagingSessionDidStop did not fire on disconnect #2372
  2. Add backwards compatibility with aws-js-sdk 2.x for getMessagingSessionEndpoint call

Testing:

  • Tested reconnect loop fix by expiring credentials to validate reconnect loop on messagingSession breaks after the timeout

  • Tested compatibility with aws sdk v2 by changing the chime client instantiation in the messagingSession demo

  • Set aws credentials with in correct aws session token

  • 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

  • Reconnect loop is triggered but breaks after timeout

Can these tested using a demo application? Please provide reproducible step-by-step instructions.
To Test aws js sdk v2 compatibility run the messagingSession demo with below changes
Make the following changes in amazon-chime-sdk-js/demos/browser/app/messagingSession/messagingSession.ts

import { ChimeSDKMessagingClient } from '@aws-sdk/client-chime-sdk-messaging'
to
import * as Chime from 'aws-sdk/clients/chime';

and change

 const chime = new ChimeSDKMessagingClient({ region: 'us-east-1', credentials: response });
to
const chime = new Chime({ region: 'us-east-1', credentials: response });

To Test aws js sdk v3 with v2 style run the messagingSession demo with below changes
Make the following changes in amazon-chime-sdk-js/demos/browser/app/messagingSession/messagingSession.ts

import { ChimeSDKMessagingClient } from '@aws-sdk/client-chime-sdk-messaging'
to
import { ChimeSDKMessaging } from '@aws-sdk/client-chime-sdk-messaging';

and

 const chime = new ChimeSDKMessagingClient({ region: 'us-east-1', credentials: response });
to
const chime = new ChimeSDKMessaging({ region: 'us-east-1', credentials: response });

and then run
Checklist:

  1. Have you successfully run npm run build:release locally?
    yes

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

  3. 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.
    yes

@manasisurve manasisurve requested a review from a team as a code owner September 12, 2022 18:45
shi-su and others added 3 commits September 14, 2022 16:24
…2429)

This change is to follow the protobuf changes in backend including
1. Add metric type of whether encoder/decoder implementation name is in hardware.
2. Add a dimension field for stream metric frame.
3. Rename VIDEO_AVERAGE_ENCODE_MS to VIDEO_ENCODE_MS to make it consistent with VIDEO_DECODE_MS.
@dpwspoon dpwspoon self-requested a review September 15, 2022 01:04
dpwspoon
dpwspoon previously approved these changes Sep 15, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
shi-su and others added 4 commits September 19, 2022 16:43
Report encode/decode time and if encoder/decoder is in hardware, and add encoder/decoder name as a metric dimension

Co-authored-by: Shi Su <[email protected]>
Fix a confusing function name from millisecondsPerSecond to averageTimeSpentPerSecondInMillisecond.
…ckwards compatibility with AWS JS SDK V2 for getMessagingSessionEndpoint
@@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `MessagingSession` reconnect loop did not break on error past reconnect deadline. Infinite reconnect loop was caused due to firstConnectionAttemptTimestamp not being set as startedConnectionAttempt was not invoked. Check https://github.com/aws/amazon-chime-sdk-js/issues/2372 for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Back tick firstConnectionAttemptTimestamp and startedConnectionAttempt.

Copy link
Contributor

@devalevenkatesh devalevenkatesh left a comment

Choose a reason for hiding this comment

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

Is the package-lock.json changes necessary?

Does not look like from the fix that those are needed.

@manasisurve manasisurve changed the base branch from main to release-3.x September 20, 2022 00:35
@manasisurve manasisurve changed the base branch from release-3.x to main September 20, 2022 00:35
@manasisurve manasisurve dismissed devalevenkatesh’s stale review September 20, 2022 00:35

The base branch was changed.

@devalevenkatesh
Copy link
Contributor

Closing due to #2444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants