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.

Sign up for GitHub

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

fix(plugin-meetings): stop stats analyzer during reconnection #3990

Merged
merged 7 commits into from
Nov 25, 2024
Merged

fix(plugin-meetings): stop stats analyzer during reconnection #3990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
757cd07
fix: stop stats analyzer during reconnection
Nov 18, 2024
23bf8cf
Merge branch 'next' into fix-cleanup-stats-analyzer-before-full-recon…
k-wasniowski Nov 19, 2024
457fcd4
Merge branch 'next' into fix-cleanup-stats-analyzer-before-full-recon…
k-wasniowski Nov 22, 2024
27a3b93
Merge branch 'next' into fix-cleanup-stats-analyzer-before-full-recon…
k-wasniowski Nov 25, 2024
178e655
fix: move cleanup into reconnect method
Nov 25, 2024
4afdb71
fix: correct tests
k-wasniowski Nov 25, 2024
ceb7fd0
fix: add try catch
k-wasniowski Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/@webex/plugin-meetings/src/meeting/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4904,6 +4904,8 @@ export default class Meeting extends StatelessWebexPlugin {
);
}

this.cleanUpBeforeReconnection();

return this.reconnectionManager
.reconnect(options, async () => {
await this.waitForRemoteSDPAnswer();
Expand Down Expand Up @@ -7032,6 +7034,23 @@ export default class Meeting extends StatelessWebexPlugin {
}
}

private async cleanUpBeforeReconnection(): Promise<void> {
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();
}
} catch (error) {
LoggerProxy.logger.error(
'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup: ',
error
);
}
}
Comment on lines +7037 to +7052
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing error handling and cleanup sequence

The error handling and cleanup sequence in cleanUpBeforeReconnection() could be more robust:

  1. The error handling could be more specific to different types of failures
  2. The cleanup sequence could be more comprehensive
  3. Additional logging would help with debugging
 private async cleanUpBeforeReconnection(): Promise<void> {
   try {
+    LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Starting cleanup');
+
     // 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) {
+      LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Stopping stats analyzer');
       await this.statsAnalyzer.stopAnalyzer();
     }
+
+    // Additional cleanup tasks could be added here
+    LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Cleanup completed successfully');
   } catch (error) {
-    LoggerProxy.logger.error(
-      'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup: ',
-      error
-    );
+    // More specific error handling
+    if (error instanceof StatsAnalyzerError) {
+      LoggerProxy.logger.error(
+        'Meeting:index#cleanUpBeforeReconnection --> Stats analyzer cleanup failed: ',
+        error
+      );
+    } else {
+      LoggerProxy.logger.error(
+        'Meeting:index#cleanUpBeforeReconnection --> Unexpected error during cleanup: ',
+        error
+      );
+    }
+    // Re-throw to allow caller to handle the error
+    throw error;
   }
 }

Committable suggestion skipped: line range outside the PR's diff.


/**
* 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).
Expand Down
69 changes: 51 additions & 18 deletions packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,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', () => {
Expand Down Expand Up @@ -3808,12 +3811,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});
});

Expand Down Expand Up @@ -4006,7 +4009,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}`
);
Comment on lines +4012 to +4015
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Duplicate error handling code for stream readyState checks

Similar error handling code is duplicated for checking stream readyState across audio, video, screenShare audio and screenShare video streams. This should be consolidated into a reusable helper function.

// Add helper function:
+ const validateStreamReadyState = (stream, type) => {
+   if (stream?.readyState === 'ended') {
+     throw new Error(`Attempted to publish ${type} stream with ended readyState, correlationId=${meeting.correlationId}`);
+   }
+ }

// Use helper function:
- if (stream?.readyState === 'ended') {
-   assert.throws(
-     meeting.publishStreams(localStreams),
-     `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}`
-   );
- }
+ validateStreamReadyState(stream, 'microphone');

Also applies to: 4028-4031, 4044-4047, 4060-4063

} else {
assert.calledOnceWithExactly(
meeting.sendSlotManager.getSlot(MediaType.AudioMain).publishStream,
Expand All @@ -4019,7 +4025,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,
Expand All @@ -4032,7 +4041,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}`
);
Comment on lines +4044 to +4047
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor duplicate error handling code for screen share streams

Similar error handling code is duplicated for both screenShare audio and video streams. This should be consolidated with the earlier microphone stream error handling into a single reusable function.

// Use the same helper function:
const handleStreamError = (streamType, correlationId) => {
  throw new Error(`Attempted to publish ${streamType} stream with ended readyState, correlationId=${correlationId}`);
};

// Then use it consistently:
if (stream?.readyState === 'ended') {
  handleStreamError('screenShare audio', meeting.correlationId);
}
if (stream?.readyState === 'ended') {
  handleStreamError('screenShare video', meeting.correlationId);
}

Also applies to: 4047-4050

} else {
assert.calledOnceWithExactly(
meeting.sendSlotManager.getSlot(MediaType.AudioSlides).publishStream,
Expand All @@ -4045,7 +4057,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,
Expand Down Expand Up @@ -4340,14 +4355,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 () => {
Expand Down Expand Up @@ -4416,12 +4431,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', () => {
Expand Down Expand Up @@ -6998,7 +7012,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: '',
});
});
});

Expand Down Expand Up @@ -7670,11 +7687,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(() => {
Expand Down Expand Up @@ -7930,7 +7947,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) => {
Expand Down Expand Up @@ -8005,10 +8022,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) => {
Expand Down Expand Up @@ -8038,6 +8055,22 @@ 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();
meeting.reconnectionManager = {
reconnect: sinon.stub().resolves(),
resetReconnectionTimer: () => {}
};
meeting.currentMediaStatus = {
video: true
};

await mockFailedEvent();

assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer);
});
});

describe('should send correct metrics for ROAP_FAILURE event', () => {
Expand Down
Loading