fix: Avoid potential call-manager race condition (#2508)
This commit is contained in:
committed by
dermotduffy
parent
773cee95a5
commit
ef9c9c8d5c
@@ -15,6 +15,15 @@ export class CallManager {
|
|||||||
private _ringtone = new Ringtone();
|
private _ringtone = new Ringtone();
|
||||||
private _unansweredTimer = new Timer();
|
private _unansweredTimer = new Timer();
|
||||||
|
|
||||||
|
// Identifies the current init/uninit cycle so an in-flight `start()`
|
||||||
|
// resuming from its microphone-connect await can detect that its CallManager
|
||||||
|
// was torn down -- or torn down and re-initialized -- while it was suspended
|
||||||
|
// and bail before installing a session or ringtone. Without this guard the
|
||||||
|
// resumed tail leaks audio onto the shared lock from an instance the user
|
||||||
|
// can no longer see or control, and may install state into a fresh
|
||||||
|
// lifecycle from a request that belongs to the previous one.
|
||||||
|
private _initEpoch = 0;
|
||||||
|
|
||||||
constructor(api: CardCallAPI) {
|
constructor(api: CardCallAPI) {
|
||||||
this._api = api;
|
this._api = api;
|
||||||
}
|
}
|
||||||
@@ -94,7 +103,19 @@ export class CallManager {
|
|||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!(await this._connectMicrophone(inbound))) {
|
const initEpoch = this._initEpoch;
|
||||||
|
const microphoneConnected = await this._connectMicrophone();
|
||||||
|
// If the init/uninit lifecycle advanced while the microphone connect was
|
||||||
|
// in flight, this request belongs to a previous lifecycle -- the view we
|
||||||
|
// captured may be stale, the triggering context is gone, and there is no
|
||||||
|
// clean teardown path for state we'd install here. Bail before touching
|
||||||
|
// `_call`, the ringtone lock, or surfacing a notification onto a torn-down
|
||||||
|
// NotificationManager.
|
||||||
|
if (initEpoch !== this._initEpoch) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
if (!microphoneConnected) {
|
||||||
|
this._notifyError('error.call_microphone_forbidden', inbound);
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -220,6 +241,7 @@ export class CallManager {
|
|||||||
//
|
//
|
||||||
// Safe to re-initialize afterwards via `initialize()`.
|
// Safe to re-initialize afterwards via `initialize()`.
|
||||||
public uninitialize(): void {
|
public uninitialize(): void {
|
||||||
|
this._initEpoch++;
|
||||||
this._ringtone.stop();
|
this._ringtone.stop();
|
||||||
this._unansweredTimer.stop();
|
this._unansweredTimer.stop();
|
||||||
if (this._call) {
|
if (this._call) {
|
||||||
@@ -343,7 +365,7 @@ export class CallManager {
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
private async _connectMicrophone(inbound: boolean): Promise<boolean> {
|
private async _connectMicrophone(): Promise<boolean> {
|
||||||
const microphoneManager = this._api.getMicrophoneManager();
|
const microphoneManager = this._api.getMicrophoneManager();
|
||||||
if (microphoneManager.isConnected()) {
|
if (microphoneManager.isConnected()) {
|
||||||
return true;
|
return true;
|
||||||
@@ -352,7 +374,6 @@ export class CallManager {
|
|||||||
await microphoneManager.connect();
|
await microphoneManager.connect();
|
||||||
return true;
|
return true;
|
||||||
} catch {
|
} catch {
|
||||||
this._notifyError('error.call_microphone_forbidden', inbound);
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1609,3 +1609,76 @@ describe('session end during setState', () => {
|
|||||||
expect(manager.isActive()).toBe(false);
|
expect(manager.isActive()).toBe(false);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('uninitialize during in-flight start', () => {
|
||||||
|
it('should not install a session or ring when uninitialized mid-await', async () => {
|
||||||
|
const api = createAPI({
|
||||||
|
view: createView({ camera: 'camera.office' }),
|
||||||
|
microphoneConnected: false,
|
||||||
|
config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
|
||||||
|
});
|
||||||
|
let resolveConnect: () => void = () => {};
|
||||||
|
vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
|
||||||
|
new Promise<void>((resolve) => {
|
||||||
|
resolveConnect = resolve;
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
const manager = new CallManager(api);
|
||||||
|
manager.initialize();
|
||||||
|
|
||||||
|
const startPromise = manager.start({ inbound: true });
|
||||||
|
manager.uninitialize();
|
||||||
|
resolveConnect();
|
||||||
|
|
||||||
|
expect(await startPromise).toBe(false);
|
||||||
|
expect(getRingtone().start).not.toBeCalled();
|
||||||
|
expect(manager.isActive()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should not install a session or ring when uninitialized and re-initialized mid-await', async () => {
|
||||||
|
const api = createAPI({
|
||||||
|
view: createView({ camera: 'camera.office' }),
|
||||||
|
microphoneConnected: false,
|
||||||
|
config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
|
||||||
|
});
|
||||||
|
let resolveConnect: () => void = () => {};
|
||||||
|
vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
|
||||||
|
new Promise<void>((resolve) => {
|
||||||
|
resolveConnect = resolve;
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
const manager = new CallManager(api);
|
||||||
|
manager.initialize();
|
||||||
|
|
||||||
|
const startPromise = manager.start({ inbound: true });
|
||||||
|
manager.uninitialize();
|
||||||
|
manager.initialize();
|
||||||
|
resolveConnect();
|
||||||
|
|
||||||
|
expect(await startPromise).toBe(false);
|
||||||
|
expect(getRingtone().start).not.toBeCalled();
|
||||||
|
expect(manager.isActive()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should suppress the microphone-failure notification when uninitialized mid-await', async () => {
|
||||||
|
const api = createAPI({
|
||||||
|
view: createView({ camera: 'camera.office' }),
|
||||||
|
microphoneConnected: false,
|
||||||
|
});
|
||||||
|
let rejectConnect: (reason: unknown) => void = () => {};
|
||||||
|
vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
|
||||||
|
new Promise<void>((_, reject) => {
|
||||||
|
rejectConnect = reject;
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
const manager = new CallManager(api);
|
||||||
|
manager.initialize();
|
||||||
|
|
||||||
|
const startPromise = manager.start();
|
||||||
|
manager.uninitialize();
|
||||||
|
rejectConnect(new Error('denied'));
|
||||||
|
|
||||||
|
expect(await startPromise).toBe(false);
|
||||||
|
expect(api.getNotificationManager().setNotification).not.toBeCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user