From 757cd07d615ccaadd4a81fb83d88522ef4bfc71d Mon Sep 17 00:00:00 2001 From: kwasniow Date: Mon, 18 Nov 2024 14:39:45 +0100 Subject: [PATCH 1/4] fix: stop stats analyzer during reconnection --- .../plugin-meetings/src/meeting/index.ts | 12 ++++ .../test/unit/spec/meeting/index.js | 62 +++++++++++++------ 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/packages/@webex/plugin-meetings/src/meeting/index.ts b/packages/@webex/plugin-meetings/src/meeting/index.ts index 425d67cb0b4..eec7a2633ad 100644 --- a/packages/@webex/plugin-meetings/src/meeting/index.ts +++ b/packages/@webex/plugin-meetings/src/meeting/index.ts @@ -6062,6 +6062,8 @@ export default class Meeting extends StatelessWebexPlugin { // so reset the timer as it's not needed anymore, we want to reconnect immediately this.reconnectionManager.resetReconnectionTimer(); + this.cleanUpBeforeReconnection(); + this.reconnect({networkDisconnect: true}); this.uploadLogs({ @@ -6939,6 +6941,16 @@ export default class Meeting extends StatelessWebexPlugin { } } + private async cleanUpBeforeReconnection(): Promise { + // when media fails, we want to upload a webrtc dump to see whats going on + // this function is async, but returns once the stats have been gathered + await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); + + if (this.statsAnalyzer) { + await this.statsAnalyzer.stopAnalyzer(); + } + } + /** * Creates an instance of LocusMediaRequest for this meeting - it is needed for doing any calls * to Locus /media API (these are used for sending Roap messages and updating audio/video mute status). diff --git a/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js b/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js index 3c9c8b8d437..d43ce69cb8b 100644 --- a/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js +++ b/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js @@ -375,7 +375,10 @@ describe('plugin-meetings', () => { } ); assert.equal(newMeeting.correlationId, newMeeting.id); - assert.deepEqual(newMeeting.callStateForMetrics, {correlationId: newMeeting.id, sessionCorrelationId: ''}); + assert.deepEqual(newMeeting.callStateForMetrics, { + correlationId: newMeeting.id, + sessionCorrelationId: '', + }); }); it('correlationId can be provided in callStateForMetrics', () => { @@ -3793,12 +3796,12 @@ describe('plugin-meetings', () => { id: 'fake locus from mocked join request', locusUrl: 'fake locus url', mediaId: 'fake media id', - }) + }); sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ headers: { trackingid: 'fake tracking id', - } - }) + }, + }); await meeting.join({enableMultistream: isMultistream}); }); @@ -3991,7 +3994,10 @@ describe('plugin-meetings', () => { assert.notCalled( meeting.sendSlotManager.getSlot(MediaType.AudioMain).publishStream ); - assert.throws(meeting.publishStreams(localStreams), `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}`); + assert.throws( + meeting.publishStreams(localStreams), + `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}` + ); } else { assert.calledOnceWithExactly( meeting.sendSlotManager.getSlot(MediaType.AudioMain).publishStream, @@ -4004,7 +4010,10 @@ describe('plugin-meetings', () => { assert.notCalled( meeting.sendSlotManager.getSlot(MediaType.VideoMain).publishStream ); - assert.throws(meeting.publishStreams(localStreams), `Attempted to publish camera stream with ended readyState, correlationId=${meeting.correlationId}`); + assert.throws( + meeting.publishStreams(localStreams), + `Attempted to publish camera stream with ended readyState, correlationId=${meeting.correlationId}` + ); } else { assert.calledOnceWithExactly( meeting.sendSlotManager.getSlot(MediaType.VideoMain).publishStream, @@ -4017,7 +4026,10 @@ describe('plugin-meetings', () => { assert.notCalled( meeting.sendSlotManager.getSlot(MediaType.AudioSlides).publishStream ); - assert.throws(meeting.publishStreams(localStreams), `Attempted to publish screenShare audio stream with ended readyState, correlationId=${meeting.correlationId}`); + assert.throws( + meeting.publishStreams(localStreams), + `Attempted to publish screenShare audio stream with ended readyState, correlationId=${meeting.correlationId}` + ); } else { assert.calledOnceWithExactly( meeting.sendSlotManager.getSlot(MediaType.AudioSlides).publishStream, @@ -4030,7 +4042,10 @@ describe('plugin-meetings', () => { assert.notCalled( meeting.sendSlotManager.getSlot(MediaType.VideoSlides).publishStream ); - assert.throws(meeting.publishStreams(localStreams), `Attempted to publish screenShare video stream with ended readyState, correlationId=${meeting.correlationId}`); + assert.throws( + meeting.publishStreams(localStreams), + `Attempted to publish screenShare video stream with ended readyState, correlationId=${meeting.correlationId}` + ); } else { assert.calledOnceWithExactly( meeting.sendSlotManager.getSlot(MediaType.VideoSlides).publishStream, @@ -4325,14 +4340,14 @@ describe('plugin-meetings', () => { const handleDeviceLoggingSpy = sinon.spy(Meeting, 'handleDeviceLogging'); await meeting.addMedia({audioEnabled: false}); //calling handleDeviceLogging with audioEnaled as true adn videoEnabled as false - assert.calledWith(handleDeviceLoggingSpy,false,true); + assert.calledWith(handleDeviceLoggingSpy, false, true); }); it('addMedia() works correctly when video is disabled with no streams to publish', async () => { const handleDeviceLoggingSpy = sinon.spy(Meeting, 'handleDeviceLogging'); await meeting.addMedia({videoEnabled: false}); //calling handleDeviceLogging audioEnabled as true videoEnabled as false - assert.calledWith(handleDeviceLoggingSpy,true,false); + assert.calledWith(handleDeviceLoggingSpy, true, false); }); it('addMedia() works correctly when video is disabled with no streams to publish', async () => { @@ -4401,12 +4416,11 @@ describe('plugin-meetings', () => { assert.calledTwice(locusMediaRequestStub); }); - it('addMedia() works correctly when both shareAudio and shareVideo is disabled with no streams publish', async () => { const handleDeviceLoggingSpy = sinon.spy(Meeting, 'handleDeviceLogging'); await meeting.addMedia({shareAudioEnabled: false, shareVideoEnabled: false}); //calling handleDeviceLogging with audioEnabled true and videoEnabled as true - assert.calledWith(handleDeviceLoggingSpy,true,true); + assert.calledWith(handleDeviceLoggingSpy, true, true); }); describe('publishStreams()/unpublishStreams() calls', () => { @@ -6967,7 +6981,10 @@ describe('plugin-meetings', () => { assert.deepEqual(meeting.callStateForMetrics, {correlationId, sessionCorrelationId: ''}); meeting.setCorrelationId(uuid1); assert.equal(meeting.correlationId, uuid1); - assert.deepEqual(meeting.callStateForMetrics, {correlationId: uuid1, sessionCorrelationId: ''}); + assert.deepEqual(meeting.callStateForMetrics, { + correlationId: uuid1, + sessionCorrelationId: '', + }); }); }); @@ -7639,11 +7656,11 @@ describe('plugin-meetings', () => { id: 'stream', getTracks: () => [{id: 'track', addEventListener: sinon.stub()}], }; - const simulateConnectionStateChange = (newState) => { + const simulateConnectionStateChange = async (newState) => { meeting.mediaProperties.webrtcMediaConnection.getConnectionState = sinon .stub() .returns(newState); - eventListeners[MediaConnectionEventNames.PEER_CONNECTION_STATE_CHANGED](); + await eventListeners[MediaConnectionEventNames.PEER_CONNECTION_STATE_CHANGED](); }; beforeEach(() => { @@ -7899,7 +7916,7 @@ describe('plugin-meetings', () => { meeting.reconnectionManager = new ReconnectionManager(meeting); meeting.reconnectionManager.iceReconnected = sinon.stub().returns(undefined); meeting.setNetworkStatus = sinon.stub().returns(undefined); - meeting.statsAnalyzer = {startAnalyzer: sinon.stub()}; + meeting.statsAnalyzer = {startAnalyzer: sinon.stub(), stopAnalyzer: sinon.stub()}; meeting.mediaProperties.webrtcMediaConnection = { // mock the on() method and store all the listeners on: sinon.stub().callsFake((event, listener) => { @@ -7974,10 +7991,10 @@ describe('plugin-meetings', () => { }); describe('CONNECTION_STATE_CHANGED event when state = "Failed"', () => { - const mockFailedEvent = () => { + const mockFailedEvent = async () => { meeting.setupMediaConnectionListeners(); - simulateConnectionStateChange(ConnectionState.Failed); + await simulateConnectionStateChange(ConnectionState.Failed); }; const checkBehavioralMetricSent = (hasMediaConnectionConnectedAtLeastOnce = false) => { @@ -8007,6 +8024,15 @@ describe('plugin-meetings', () => { assert.notCalled(webex.internal.newMetrics.submitClientEvent); checkBehavioralMetricSent(true); }); + + it('stop stats analyzer during reconnection ', async () => { + meeting.hasMediaConnectionConnectedAtLeastOnce = true; + meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); + + await mockFailedEvent(); + + assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); + }); }); describe('should send correct metrics for ROAP_FAILURE event', () => { From 178e6553439b83fec8d0d19b74061e7b0daa8765 Mon Sep 17 00:00:00 2001 From: kwasniow Date: Mon, 25 Nov 2024 11:17:31 +0100 Subject: [PATCH 2/4] fix: move cleanup into reconnect method --- packages/@webex/plugin-meetings/src/meeting/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@webex/plugin-meetings/src/meeting/index.ts b/packages/@webex/plugin-meetings/src/meeting/index.ts index a844bc2cd8f..ef23e3feec1 100644 --- a/packages/@webex/plugin-meetings/src/meeting/index.ts +++ b/packages/@webex/plugin-meetings/src/meeting/index.ts @@ -4904,6 +4904,8 @@ export default class Meeting extends StatelessWebexPlugin { ); } + this.cleanUpBeforeReconnection(); + return this.reconnectionManager .reconnect(options, async () => { await this.waitForRemoteSDPAnswer(); @@ -6155,8 +6157,6 @@ export default class Meeting extends StatelessWebexPlugin { // so reset the timer as it's not needed anymore, we want to reconnect immediately this.reconnectionManager.resetReconnectionTimer(); - this.cleanUpBeforeReconnection(); - this.reconnect({networkDisconnect: true}); this.uploadLogs({ From 4afdb7197fa360b5b2269c4592bce8ceb8704549 Mon Sep 17 00:00:00 2001 From: k-wasniowski Date: Mon, 25 Nov 2024 11:54:09 +0100 Subject: [PATCH 3/4] fix: correct tests --- .../@webex/plugin-meetings/test/unit/spec/meeting/index.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js b/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js index 7f3dcaa3434..78d5fb58bc9 100644 --- a/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js +++ b/packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js @@ -8059,6 +8059,13 @@ describe('plugin-meetings', () => { it('stop stats analyzer during reconnection ', async () => { meeting.hasMediaConnectionConnectedAtLeastOnce = true; meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); + meeting.reconnectionManager = { + reconnect: sinon.stub().resolves(), + resetReconnectionTimer: () => {} + }; + meeting.currentMediaStatus = { + video: true + }; await mockFailedEvent(); From ceb7fd0f11a5cfb91222e107428e9c5dcec4f3e5 Mon Sep 17 00:00:00 2001 From: k-wasniowski Date: Mon, 25 Nov 2024 13:10:38 +0100 Subject: [PATCH 4/4] fix: add try catch --- .../@webex/plugin-meetings/src/meeting/index.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/@webex/plugin-meetings/src/meeting/index.ts b/packages/@webex/plugin-meetings/src/meeting/index.ts index ef23e3feec1..a1cd18ad353 100644 --- a/packages/@webex/plugin-meetings/src/meeting/index.ts +++ b/packages/@webex/plugin-meetings/src/meeting/index.ts @@ -7035,12 +7035,19 @@ export default class Meeting extends StatelessWebexPlugin { } private async cleanUpBeforeReconnection(): Promise { - // when media fails, we want to upload a webrtc dump to see whats going on - // this function is async, but returns once the stats have been gathered - await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); + try { + // when media fails, we want to upload a webrtc dump to see whats going on + // this function is async, but returns once the stats have been gathered + await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); - if (this.statsAnalyzer) { - await this.statsAnalyzer.stopAnalyzer(); + if (this.statsAnalyzer) { + await this.statsAnalyzer.stopAnalyzer(); + } + } catch (error) { + LoggerProxy.logger.error( + 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup: ', + error + ); } }