Add callIdHistory to context + internal context (#1817)

* - Add callIdHistory to context + internal context
- Update all visit to callId using latestCallId
- Encapsule the access to internalMap directly to ensure no leaks

Author: PorterNan

change/@internal-calling-stateful-client-cf376626-41a0-4c00-8b7b-f95b52bfa509.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "- Add callIdHistory to context + internal context - Update all visit to callId using latestCallId - Encapsule the access to internalMap directly to ensure no leaks",
+  "packageName": "@internal/calling-stateful-client",
+  "email": "jiangnanhello@live.com",
+  "dependentChangeType": "patch"
+}

packages/calling-stateful-client/src/CallAgentDeclarative.test.ts

@@ -214,7 +214,7 @@ describe('declarative call agent', () => {
     mockCallAgent.calls = [];
     callAgentDeclaratify(mockCallAgent, context, internalContext);
     expect(Object.keys(context.getState().calls).length).toBe(0);
-    expect(internalContext.getRemoteRenderInfos().size).toBe(0);
+    expect(Array.from(internalContext.getCallIds()).length).toBe(0);
   });
 
   test('should update state with new call when startCall is invoked', () => {

packages/calling-stateful-client/src/CallContext.ts

@@ -26,6 +26,7 @@ import {
   CallError
 } from './CallClientState';
 import { callingStatefulLogger } from './Logger';
+import { CallIdHistory } from './CallIdHistory';
 
 enableMapSet();
 // Needed to generate state diff for verbose logging.
@@ -45,6 +46,7 @@ export class CallContext {
   private _state: CallClientState;
   private _emitter: EventEmitter;
   private _atomicId: number;
+  private _callIdHistory: CallIdHistory = new CallIdHistory();
 
   constructor(userId: CommunicationIdentifierKind, maxListeners = 50) {
     this._logger = createClientLogger('communication-react:calling-context');
@@ -114,7 +116,8 @@ export class CallContext {
 
   public setCall(call: CallState): void {
     this.modifyState((draft: CallClientState) => {
-      const existingCall = draft.calls[call.id];
+      const latestCallId = this._callIdHistory.latestCallId(call.id);
+      const existingCall = draft.calls[latestCallId];
       if (existingCall) {
         existingCall.callerInfo = call.callerInfo;
         existingCall.state = call.state;
@@ -128,44 +131,46 @@ export class CallContext {
         existingCall.recording.isRecordingActive = call.recording.isRecordingActive;
         // We don't update the startTime and endTime if we are updating an existing active call
       } else {
-        draft.calls[call.id] = call;
+        draft.calls[latestCallId] = call;
       }
     });
   }
 
   public removeCall(callId: string): void {
     this.modifyState((draft: CallClientState) => {
-      delete draft.calls[callId];
+      delete draft.calls[this._callIdHistory.latestCallId(callId)];
     });
   }
 
   public setCallEnded(callId: string, callEndReason: CallEndReason | undefined): void {
+    const latestCallId = this._callIdHistory.latestCallId(callId);
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[latestCallId];
       if (call) {
         call.endTime = new Date();
         call.callEndReason = callEndReason;
-        delete draft.calls[callId];
+        delete draft.calls[latestCallId];
         // Performance note: This loop should run only once because the number of entries
         // is never allowed to exceed MAX_CALL_HISTORY_LENGTH. A loop is used for correctness.
         while (Object.keys(draft.callsEnded).length >= MAX_CALL_HISTORY_LENGTH) {
           delete draft.callsEnded[findOldestCallEnded(draft.callsEnded)];
         }
-        draft.callsEnded[callId] = call;
+        draft.callsEnded[latestCallId] = call;
       }
     });
   }
 
   public setCallState(callId: string, state: CallStatus): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.state = state;
       }
     });
   }
 
   public setCallId(newCallId: string, oldCallId: string): void {
+    this._callIdHistory.updateCallIdHistory(newCallId, oldCallId);
     this.modifyState((draft: CallClientState) => {
       const call = draft.calls[oldCallId];
       if (call) {
@@ -177,7 +182,7 @@ export class CallContext {
 
   public setCallIsScreenSharingOn(callId: string, isScreenSharingOn: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.isScreenSharingOn = isScreenSharingOn;
       }
@@ -190,7 +195,7 @@ export class CallContext {
     removeRemoteParticipant: string[]
   ): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         removeRemoteParticipant.forEach((id: string) => {
           delete call.remoteParticipants[id];
@@ -208,7 +213,7 @@ export class CallContext {
     removeRemoteParticipant: string[]
   ): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         removeRemoteParticipant.forEach((id: string) => {
           delete call.remoteParticipantsEnded[id];
@@ -222,7 +227,7 @@ export class CallContext {
 
   public setCallLocalVideoStream(callId: string, streams: LocalVideoStreamState[]): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.localVideoStreams = streams;
       }
@@ -231,7 +236,7 @@ export class CallContext {
 
   public setCallIsMicrophoneMuted(callId: string, isMicrophoneMuted: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.isMuted = isMicrophoneMuted;
       }
@@ -240,7 +245,7 @@ export class CallContext {
 
   public setCallDominantSpeakers(callId: string, dominantSpeakers: DominantSpeakersInfo): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.dominantSpeakers = dominantSpeakers;
       }
@@ -249,7 +254,7 @@ export class CallContext {
 
   public setCallRecordingActive(callId: string, isRecordingActive: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.recording.isRecordingActive = isRecordingActive;
       }
@@ -258,7 +263,7 @@ export class CallContext {
 
   public setCallTranscriptionActive(callId: string, isTranscriptionActive: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.transcription.isTranscriptionActive = isTranscriptionActive;
       }
@@ -267,7 +272,7 @@ export class CallContext {
 
   public setCallScreenShareParticipant(callId: string, participantKey: string | undefined): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         call.screenShareRemoteParticipant = participantKey;
       }
@@ -276,7 +281,7 @@ export class CallContext {
 
   public setLocalVideoStreamRendererView(callId: string, view: VideoStreamRendererViewState | undefined): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         if (call.localVideoStreams.length > 0) {
           call.localVideoStreams[0].view = view;
@@ -287,7 +292,7 @@ export class CallContext {
 
   public setParticipantState(callId: string, participantKey: string, state: RemoteParticipantStatus): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -299,7 +304,7 @@ export class CallContext {
 
   public setParticipantIsMuted(callId: string, participantKey: string, muted: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -311,7 +316,7 @@ export class CallContext {
 
   public setParticipantDisplayName(callId: string, participantKey: string, displayName: string): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -323,7 +328,7 @@ export class CallContext {
 
   public setParticipantIsSpeaking(callId: string, participantKey: string, isSpeaking: boolean): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -335,7 +340,7 @@ export class CallContext {
 
   public setParticipantVideoStream(callId: string, participantKey: string, stream: RemoteVideoStreamState): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -360,7 +365,7 @@ export class CallContext {
     isAvailable: boolean
   ): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -380,7 +385,7 @@ export class CallContext {
     removeRemoteVideoStream: number[]
   ): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {
@@ -411,7 +416,7 @@ export class CallContext {
     view: VideoStreamRendererViewState | undefined
   ): void {
     this.modifyState((draft: CallClientState) => {
-      const call = draft.calls[callId];
+      const call = draft.calls[this._callIdHistory.latestCallId(callId)];
       if (call) {
         const participant = call.remoteParticipants[participantKey];
         if (participant) {

packages/calling-stateful-client/src/CallIdHistory.ts

@@ -0,0 +1,39 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ * @private
+ * Call Id will change during the call for at least 1 time
+ * This is to avoid async bug that call id has been changed during an async-await function
+ * but the function still uses stale call id to access state
+ */
+export class CallIdHistory {
+  private _callIdHistory = new Map<string, string>();
+
+  public updateCallIdHistory(newCallId: string, oldCallId: string): void {
+    // callId for a call can fluctuate between some set of values.
+    // But if a newCallId already exists, and maps to different call, we're in trouble.
+    // This can only happen if a callId is reused across two distinct calls.
+    const existing = this._callIdHistory.get(newCallId);
+    if (existing !== undefined && this.latestCallId(newCallId) !== oldCallId) {
+      console.trace(`${newCallId} alredy exists and maps to ${existing}, which is not the same as ${oldCallId}`);
+    }
+
+    // The latest callId never maps to another callId.
+    this._callIdHistory.delete(newCallId);
+    this._callIdHistory.set(oldCallId, newCallId);
+  }
+
+  public latestCallId(callId: string): string {
+    let latest = callId;
+    /* eslint no-constant-condition: ["error", { "checkLoops": false }] */
+    while (true) {
+      const newer = this._callIdHistory.get(latest);
+      if (newer === undefined) {
+        break;
+      }
+      latest = newer;
+    }
+    return latest;
+  }
+}

packages/calling-stateful-client/src/InternalCallContext.ts

@@ -3,6 +3,7 @@
 
 import { LocalVideoStream, RemoteVideoStream, VideoStreamRenderer } from '@azure/communication-calling';
 import { LocalVideoStreamState } from './CallClientState';
+import { CallIdHistory } from './CallIdHistory';
 
 /**
  * Internally tracked render status. Stores the status of a video render of a stream as rendering could take a long
@@ -50,8 +51,10 @@ export class InternalCallContext {
   // Used for keeping track of rendered LocalVideoStreams that are not part of a Call.
   // The key is the stream ID. We assume each stream ID to only have one owning render info
   private _unparentedRenderInfos = new Map<string, LocalRenderInfo>();
+  private _callIdHistory = new CallIdHistory();
 
   public setCallId(newCallId: string, oldCallId: string): void {
+    this._callIdHistory.updateCallIdHistory(newCallId, oldCallId);
     const remoteRenderInfos = this._remoteRenderInfos.get(oldCallId);
     if (remoteRenderInfos) {
       this._remoteRenderInfos.delete(oldCallId);
@@ -65,20 +68,20 @@ export class InternalCallContext {
     }
   }
 
-  public getRemoteRenderInfos(): Map<string, Map<string, Map<number, RemoteRenderInfo>>> {
-    return this._remoteRenderInfos;
+  public getCallIds(): IterableIterator<string> {
+    return this._remoteRenderInfos.keys();
   }
 
   public getRemoteRenderInfoForCall(callId: string): Map<string, Map<number, RemoteRenderInfo>> | undefined {
-    return this._remoteRenderInfos.get(callId);
+    return this._remoteRenderInfos.get(this._callIdHistory.latestCallId(callId));
   }
 
   public getRemoteRenderInfoForParticipant(
     callId: string,
     participantKey: string,
     streamId: number
   ): RemoteRenderInfo | undefined {
-    const callRenderInfos = this._remoteRenderInfos.get(callId);
+    const callRenderInfos = this._remoteRenderInfos.get(this._callIdHistory.latestCallId(callId));
     if (!callRenderInfos) {
       return undefined;
     }
@@ -97,10 +100,10 @@ export class InternalCallContext {
     status: RenderStatus,
     renderer: VideoStreamRenderer | undefined
   ): void {
-    let callRenderInfos = this._remoteRenderInfos.get(callId);
+    let callRenderInfos = this._remoteRenderInfos.get(this._callIdHistory.latestCallId(callId));
     if (!callRenderInfos) {
       callRenderInfos = new Map<string, Map<number, RemoteRenderInfo>>();
-      this._remoteRenderInfos.set(callId, callRenderInfos);
+      this._remoteRenderInfos.set(this._callIdHistory.latestCallId(callId), callRenderInfos);
     }
 
     let participantRenderInfos = callRenderInfos.get(participantKey);
@@ -113,7 +116,7 @@ export class InternalCallContext {
   }
 
   public deleteRemoteRenderInfo(callId: string, participantKey: string, streamId: number): void {
-    const callRenderInfos = this._remoteRenderInfos.get(callId);
+    const callRenderInfos = this._remoteRenderInfos.get(this._callIdHistory.latestCallId(callId));
     if (!callRenderInfos) {
       return;
     }
@@ -132,15 +135,15 @@ export class InternalCallContext {
     status: RenderStatus,
     renderer: VideoStreamRenderer | undefined
   ): void {
-    this._localRenderInfos.set(callId, { stream, status, renderer });
+    this._localRenderInfos.set(this._callIdHistory.latestCallId(callId), { stream, status, renderer });
   }
 
   public getLocalRenderInfo(callId: string): LocalRenderInfo | undefined {
-    return this._localRenderInfos.get(callId);
+    return this._localRenderInfos.get(this._callIdHistory.latestCallId(callId));
   }
 
   public deleteLocalRenderInfo(callId: string): void {
-    this._localRenderInfos.delete(callId);
+    this._localRenderInfos.delete(this._callIdHistory.latestCallId(callId));
   }
 
   public getUnparentedRenderInfo(localVideoStream: LocalVideoStreamState): LocalRenderInfo | undefined {

packages/calling-stateful-client/src/StreamUtils.ts

@@ -451,8 +451,8 @@ export function disposeAllViewsFromCall(
  * @private
  */
 export function disposeAllViews(context: CallContext, internalContext: InternalCallContext): void {
-  const remoteStreamAndRenderers = internalContext.getRemoteRenderInfos();
-  for (const [callId] of remoteStreamAndRenderers.entries()) {
+  const callIds = internalContext.getCallIds();
+  for (const callId of callIds) {
     disposeAllViewsFromCall(context, internalContext, callId);
   }
 }