Skip to content

fix(plugin-meetings): stop reachability when joining a meeting #4141

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

fix(plugin-meetings): stop reachability when joining a meeting #4141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 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 @@ -6719,6 +6719,9 @@ export default class Meeting extends StatelessWebexPlugin {
new RtcMetrics(this.webex, {meetingId: this.id}, this.correlationId)
: undefined;

// ongoing reachability checks slow down new media connections especially on Firefox, so we stop them
this.getWebexObject().meetings.reachability.stopReachability();
Copy link
Contributor

@sreenara sreenara Mar 11, 2025

Choose a reason for hiding this comment

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

Quick question. Why do we have this.getWebexObject() here but I'm seeing instances of this.webex in other places, for example the line above (6719).

Copy link
Collaborator Author

@marcin-bazyl marcin-bazyl Mar 11, 2025

Choose a reason for hiding this comment

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

it's really the same thing, I think getWebexObject() was added later, so some newer code calls it, older code uses this.webex. TBH, I'm not sure why the function was added in the first place


const mc = Media.createMediaConnection(
this.isMultistream,
this.getMediaConnectionDebugId(),
Expand Down
30 changes: 29 additions & 1 deletion packages/@webex/plugin-meetings/src/reachability/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,32 @@ export default class Reachability extends EventsScope {
}
}

/**
* Stops all reachability checks that are in progress
* @public
* @memberof Reachability
* @returns {void}
*/
public stopReachability() {
// overallTimer is always there only if there is reachability in progress
if (this.overallTimer) {
LoggerProxy.logger.log(
'Reachability:index#stopReachability --> stopping reachability checks'
);
this.abortCurrentChecks();
this.emit(
{
file: 'reachability',
function: 'stopReachability',
},
'reachability:stopped',
{}
);
this.sendMetric(true);
this.resolveReachabilityPromise();
}
}

/**
* Returns statistics about last reachability results. The returned value is an object
* with a flat list of properties so that it can be easily sent with metrics
Expand Down Expand Up @@ -637,9 +663,10 @@ export default class Reachability extends EventsScope {
/**
* Sends a metric with all the statistics about how long reachability took
*
* @param {boolean} aborted true if the reachability checks were aborted
* @returns {void}
*/
protected async sendMetric() {
protected async sendMetric(aborted = false) {
const results = [];

Object.values(this.clusterReachability).forEach((clusterReachability) => {
Expand All @@ -650,6 +677,7 @@ export default class Reachability extends EventsScope {
});

const stats = {
aborted,
vmn: {
udp: this.getStatistics(results, 'udp', true),
},
Expand Down
7 changes: 7 additions & 0 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 @@ -246,6 +246,7 @@ describe('plugin-meetings', () => {
isAnyPublicClusterReachable: sinon.stub().resolves(true),
getReachabilityResults: sinon.stub().resolves(undefined),
getReachabilityMetrics: sinon.stub().resolves({}),
stopReachability: sinon.stub(),
};
webex.internal.llm.on = sinon.stub();
webex.internal.newMetrics.callDiagnosticLatencies = new CallDiagnosticLatencies(
Expand Down Expand Up @@ -2095,6 +2096,7 @@ describe('plugin-meetings', () => {
someReachabilityMetric1: 'some value1',
someReachabilityMetric2: 'some value2',
}),
stopReachability: sinon.stub(),
};

const forceRtcMetricsSend = sinon.stub().resolves();
Expand Down Expand Up @@ -2514,6 +2516,7 @@ describe('plugin-meetings', () => {
assert.calledOnce(meeting.setMercuryListener);
assert.calledOnce(fakeMediaConnection.initiateOffer);
assert.equal(meeting.allowMediaInLobby, allowMediaInLobby);
assert.calledOnce(webex.meetings.reachability.stopReachability);
};

it('should attach the media and return promise', async () => {
Expand Down Expand Up @@ -2709,6 +2712,7 @@ describe('plugin-meetings', () => {
webex.meetings.reachability = {
isWebexMediaBackendUnreachable: sinon.stub().resolves(false),
getReachabilityMetrics: sinon.stub().resolves(),
stopReachability: sinon.stub(),
};
const MOCK_CLIENT_ERROR_CODE = 2004;
const generateClientErrorCodeForIceFailureStub = sinon
Expand Down Expand Up @@ -2917,6 +2921,7 @@ describe('plugin-meetings', () => {
.onCall(2)
.resolves(false),
getReachabilityMetrics: sinon.stub().resolves({}),
stopReachability: sinon.stub(),
};
const getErrorPayloadForClientErrorCodeStub =
(webex.internal.newMetrics.callDiagnosticMetrics.getErrorPayloadForClientErrorCode =
Expand Down Expand Up @@ -3211,6 +3216,7 @@ describe('plugin-meetings', () => {
someReachabilityMetric1: 'some value1',
someReachabilityMetric2: 'some value2',
}),
stopReachability: sinon.stub(),
};
meeting.iceCandidatesCount = 3;
meeting.iceCandidateErrors.set('701_error', 3);
Expand Down Expand Up @@ -3715,6 +3721,7 @@ describe('plugin-meetings', () => {

webex.meetings.reachability = {
isWebexMediaBackendUnreachable: sinon.stub().resolves(unreachable || false),
stopReachability: sinon.stub(),
};

const generateClientErrorCodeForIceFailureStub = sinon
Expand Down
130 changes: 120 additions & 10 deletions packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,7 @@ describe('gatherReachability', () => {

// the metrics related to ipver and trigger are not tested in these tests and are all the same, so setting them up here
const expectedMetricsFull = {
aborted: false,
...expectedMetrics,
ipver_firstIpV4: -1,
ipver_firstIpV6: -1,
Expand Down Expand Up @@ -1238,6 +1239,7 @@ describe('gatherReachability', () => {

// finally, check the metrics - they should contain values from ipNetworkDetector
assert.calledWith(Metrics.sendBehavioralMetric, 'js_sdk_reachability_completed', {
aborted: false,
vmn_udp_min: -1,
vmn_udp_max: -1,
vmn_udp_average: -1,
Expand Down Expand Up @@ -1622,22 +1624,22 @@ describe('gatherReachability', () => {
const reachability = new Reachability(webex);

let getClustersCallCount = 0;

reachability.reachabilityRequest.getClusters = sinon.stub().callsFake(() => {
getClustersCallCount++;

if (getClustersCallCount == 1) {
throw new Error('fake error');
}

return getClustersResult;
});

const promise = reachability.gatherReachability('test');

await simulateTimeout();
await promise;

assert.equal(getClustersCallCount, 2);

assert.calledOnce(clusterReachabilityCtorStub);
Expand All @@ -1647,7 +1649,7 @@ describe('gatherReachability', () => {
const reachability = new Reachability(webex);

let getClustersCallCount = 0;

reachability.reachabilityRequest.getClusters = sinon.stub().callsFake(() => {
getClustersCallCount++;

Expand All @@ -1657,9 +1659,9 @@ describe('gatherReachability', () => {
const promise = reachability.gatherReachability('test');

await simulateTimeout();

await promise;

assert.equal(getClustersCallCount, 2);

assert.neverCalledWith(clusterReachabilityCtorStub);
Expand Down Expand Up @@ -1927,7 +1929,86 @@ describe('gatherReachability', () => {
});
});


describe('stopReachability', () => {
let reachability;
let receivedEvents;
let sendMetricSpy;

beforeEach(() => {
reachability = new Reachability(webex);

receivedEvents = {};

sendMetricSpy = sinon.stub(reachability, 'sendMetric').resolves();
});

const setListener = (event) => {
reachability.on(event, () => {
receivedEvents[event] = receivedEvents[event] + 1 || 1;
});
};
it('works as expected', async () => {
setListener('reachability:stopped');
setListener('reachability:done');
setListener('reachability:firstResultAvailable');

const mockGetClustersResult = {
clusters: {
clusterA: {
udp: ['udp-urlA'],
tcp: ['tcp-urlA'],
xtls: ['xtls-urlA'],
isVideoMesh: false,
},
clusterB: {
udp: ['udp-urlB'],
tcp: ['tcp-urlB'],
xtls: ['xtls-urlB'],
isVideoMesh: false,
},
},
joinCookie: {id: 'id'},
};

reachability.reachabilityRequest.getClusters = sinon.stub().returns(mockGetClustersResult);

const gatherReachabilityFallbackSpy = sinon.spy(reachability, 'gatherReachabilityFallback');

const resultPromise = reachability.gatherReachability('test');

await testUtils.flushPromises();

reachability.stopReachability();

await resultPromise;

// simulate a lot of time passing to check that all timers were stopped and nothing else happens
clock.tick(99000);

assert.calledOnceWithExactly(mockClusterReachabilityInstances['clusterA'].abort);
assert.calledOnceWithExactly(mockClusterReachabilityInstances['clusterB'].abort);

assert.calledOnceWithExactly(sendMetricSpy, true);

assert.equal(receivedEvents['reachability:stopped'], 1);
assert.equal(receivedEvents['reachability:done'], undefined);
assert.equal(receivedEvents['reachability:firstResultAvailable'], undefined);

assert.notCalled(gatherReachabilityFallbackSpy);
});

it('does nothing if called without reachability being started', async () => {
const reachability = new Reachability(webex);

reachability.stopReachability();

assert.notCalled(sendMetricSpy);

assert.equal(receivedEvents['reachability:stopped'], undefined);
assert.equal(receivedEvents['reachability:done'], undefined);
assert.equal(receivedEvents['reachability:firstResultAvailable'], undefined);
});
});
});

describe('getReachabilityResults', () => {
Expand Down Expand Up @@ -2489,19 +2570,18 @@ describe('getStatistics', () => {
describe('sendMetric', () => {
let webex;
let reachability;
let getStatisticsStub;

beforeEach(() => {
webex = new MockWebex();
reachability = new TestReachability(webex);

sinon.stub(Metrics, 'sendBehavioralMetric');
});

it('works as expected', async () => {
// setup stub for getStatistics to return values that show what parameters it was called with,
// this way we can verify that the correct results of calls to getStatistics are placed
// in correct data fields when sendBehavioralMetric() is called
const getStatisticsStub = sinon
getStatisticsStub = sinon
.stub(reachability, 'getStatistics')
.callsFake((results, protocol, isVideoMesh) => {
return {result: 'fake', protocol, isVideoMesh};
Expand All @@ -2522,7 +2602,13 @@ describe('sendMetric', () => {
isVideoMesh: false,
},
});
});

afterEach(() => {
sinon.restore();
});

it('works as expected', async () => {
await reachability.sendMetric();

// each call to getStatistics should be made with all the results from all fake clusterReachability:
Expand All @@ -2546,6 +2632,30 @@ describe('sendMetric', () => {
assert.alwaysCalledWith(getStatisticsStub, expectedResults, sinon.match.any, sinon.match.any);

assert.calledWith(Metrics.sendBehavioralMetric, 'js_sdk_reachability_completed', {
aborted: false,
vmn_udp_result: 'fake',
vmn_udp_protocol: 'udp',
vmn_udp_isVideoMesh: true,

public_udp_result: 'fake',
public_udp_protocol: 'udp',
public_udp_isVideoMesh: false,

public_tcp_result: 'fake',
public_tcp_protocol: 'tcp',
public_tcp_isVideoMesh: false,

public_xtls_result: 'fake',
public_xtls_protocol: 'xtls',
public_xtls_isVideoMesh: false,
});
});

it('sends metric with "aborted:true" if called with aborted=true arg', async () => {
await reachability.sendMetric(true);

assert.calledWith(Metrics.sendBehavioralMetric, 'js_sdk_reachability_completed', {
aborted: true,
vmn_udp_result: 'fake',
vmn_udp_protocol: 'udp',
vmn_udp_isVideoMesh: true,
Expand Down
Loading