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 3 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
12 changes: 12 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 @@ -6155,6 +6155,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();
k-wasniowski marked this conversation as resolved.
Show resolved Hide resolved

this.reconnect({networkDisconnect: true});

this.uploadLogs({
Expand Down Expand Up @@ -7032,6 +7034,16 @@ export default class Meeting extends StatelessWebexPlugin {
}
}

private async cleanUpBeforeReconnection(): Promise<void> {
// 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();
}
}

Copy link

@coderabbitai coderabbitai bot Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for async operations

The cleanUpBeforeReconnection() method performs async operations but doesn't handle potential errors. Consider adding try-catch blocks to handle failures gracefully.

private async cleanUpBeforeReconnection(): Promise<void> {
+ try {
    await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'});

    if (this.statsAnalyzer) {
      await this.statsAnalyzer.stopAnalyzer();
    }
+ } catch (error) {
+   LoggerProxy.logger.error(
+     'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:',
+     error
+   );
+   // Optionally rethrow if you want the error to propagate
+   throw error;
+ }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private async cleanUpBeforeReconnection(): Promise<void> {
// 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();
}
}
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
);
// Optionally rethrow if you want the error to propagate
throw error;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I agree with AI, it might be worth catching all the errors so that we don't accidentally stop reconnection from starting if something silly fails inside forceSendStatsReport or stopAnalyzer

Copy link

Choose a reason for hiding this comment

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

@marcin-bazyl, understood. I'll update the cleanUpBeforeReconnection() method to include error handling as suggested, so that any errors during cleanup don't prevent the reconnection from starting. Here's the updated method:

private async cleanUpBeforeReconnection(): Promise<void> {
  try {
    // when media fails, we want to upload a webrtc dump to see what's 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
    );
    // Optionally rethrow if you want the error to propagate
    // throw error;
  }
}

This ensures that any exceptions thrown during cleanup are caught and logged without interrupting the reconnection process.

/**
* 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
62 changes: 44 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 @@ -3795,12 +3798,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 @@ -3993,7 +3996,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 @@ -4006,7 +4012,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 @@ -4019,7 +4028,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 @@ -4032,7 +4044,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 @@ -4327,14 +4342,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 @@ -4403,12 +4418,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 @@ -6985,7 +6999,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 @@ -7657,11 +7674,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 @@ -7917,7 +7934,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 @@ -7992,10 +8009,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 @@ -8025,6 +8042,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', () => {
Expand Down
Loading