From 960a08c0663c63d4ddcc8d7100f6bfc509ee0913 Mon Sep 17 00:00:00 2001 From: Hristo Terezov Date: Fri, 25 Oct 2024 10:28:31 -0500 Subject: [PATCH] fix(conference): Make sure join waits for confernce.init. It was possible that join can be executed before conference.init have even started or we haven't reached the point ot create the initialGUMPromise. This was causing the following issues: - users stuck on the prejoin screen - participants join 2+ times in the call (we have been creating more than 1 local participants from a single page). --- conference.js | 52 ++++++++++++----- modules/UI/UI.js | 8 --- .../features/base/app/components/BaseApp.tsx | 2 +- .../base/conference/middleware.any.ts | 56 ++++++++----------- .../base/conference/middleware.web.ts | 17 +++--- react/features/base/media/reducer.ts | 20 +++++-- react/features/base/util/helpers.ts | 11 +++- react/features/conference/actions.web.ts | 7 ++- .../conference/components/web/Conference.tsx | 18 +++++- 9 files changed, 112 insertions(+), 79 deletions(-) diff --git a/conference.js b/conference.js index 3c69ba315c..e5667216fb 100644 --- a/conference.js +++ b/conference.js @@ -560,10 +560,10 @@ export default { * If prejoin page is enabled open an new connection in the background * and create local tracks. * - * @param {{ roomName: string }} options + * @param {{ roomName: string, shouldDispatchConnect }} options * @returns {Promise} */ - async init({ roomName }) { + async init({ roomName, shouldDispatchConnect }) { const state = APP.store.getState(); const initialOptions = { startAudioOnly: config.startAudioOnly, @@ -607,27 +607,49 @@ export default { const { dispatch, getState } = APP.store; const { tryCreateLocalTracks, errors } = this.createInitialLocalTracks(initialOptions); - dispatch(setInitialGUMPromise(tryCreateLocalTracks.then(async tr => { + tryCreateLocalTracks.then(async tr => { const tracks = handleInitialTracks(initialOptions, tr); this._initDeviceList(true); + const { initialGUMPromise } = getState()['features/base/media']; + if (isPrejoinPageVisible(getState())) { dispatch(gumPending([ MEDIA_TYPE.AUDIO, MEDIA_TYPE.VIDEO ], IGUMPendingState.NONE)); - dispatch(setInitialGUMPromise()); + + // Since the conference is not yet created in redux this function will execute synchronous + // which will guarantee us that the local tracks are added to redux before we proceed. initPrejoin(tracks, errors, dispatch); + + // resolve the initialGUMPromise in case connect have finished so that we can proceed to join. + if (initialGUMPromise) { + logger.debug('Resolving the initialGUM promise! (prejoinVisible=true)'); + initialGUMPromise.resolve({ + tracks, + errors + }); + } + + logger.debug('Clear the initialGUM promise! (prejoinVisible=true)'); + + // For prejoin we don't need the initial GUM promise since the tracks are already added to the store + // via initPrejoin + dispatch(setInitialGUMPromise()); } else { APP.store.dispatch(displayErrorsForCreateInitialLocalTracks(errors)); setGUMPendingStateOnFailedTracks(tracks, APP.store.dispatch); + + if (initialGUMPromise) { + logger.debug('Resolving the initialGUM promise!'); + initialGUMPromise.resolve({ + tracks, + errors + }); + } } + }); - return { - tracks, - errors - }; - }))); - - if (!isPrejoinPageVisible(getState())) { + if (shouldDispatchConnect) { logger.info('Dispatching connect from init since prejoin is not visible.'); dispatch(connect()); } @@ -2047,8 +2069,9 @@ export default { const { dispatch } = APP.store; return dispatch(getAvailableDevices()) - .then(devices => { - APP.UI.onAvailableDevicesChanged(devices); + .then(() => { + this.updateAudioIconEnabled(); + this.updateVideoIconEnabled(); }); } @@ -2213,7 +2236,8 @@ export default { return Promise.all(promises) .then(() => { - APP.UI.onAvailableDevicesChanged(filteredDevices); + this.updateAudioIconEnabled(); + this.updateVideoIconEnabled(); }); }, diff --git a/modules/UI/UI.js b/modules/UI/UI.js index 2962442d16..6b5010413c 100644 --- a/modules/UI/UI.js +++ b/modules/UI/UI.js @@ -211,14 +211,6 @@ UI.handleLastNEndpoints = function(leavingIds, enteringIds) { */ UI.setAudioLevel = (id, lvl) => VideoLayout.setAudioLevel(id, lvl); -/** - * Update list of available physical devices. - */ -UI.onAvailableDevicesChanged = function() { - APP.conference.updateAudioIconEnabled(); - APP.conference.updateVideoIconEnabled(); -}; - /** * Returns the id of the current video shown on large. * Currently used by tests (torture). diff --git a/react/features/base/app/components/BaseApp.tsx b/react/features/base/app/components/BaseApp.tsx index 47a1520776..05fb44f69b 100644 --- a/react/features/base/app/components/BaseApp.tsx +++ b/react/features/base/app/components/BaseApp.tsx @@ -79,7 +79,7 @@ export default class BaseApp

extends Component { * @see {@link #_initStorage} * @type {Promise} */ - this._init = createDeferred(); + this._init = createDeferred(); try { await this._initStorage(); diff --git a/react/features/base/conference/middleware.any.ts b/react/features/base/conference/middleware.any.ts index 2ba3c44967..72e1ae70de 100644 --- a/react/features/base/conference/middleware.any.ts +++ b/react/features/base/conference/middleware.any.ts @@ -614,37 +614,6 @@ function _setRoom({ dispatch, getState }: IStore, next: Function, action: AnyAct return next(action); } -/** - * Synchronizes local tracks from state with local tracks in JitsiConference - * instance. - * - * @param {Store} store - The redux store. - * @param {Object} action - Action object. - * @private - * @returns {Promise} - */ -function _syncConferenceLocalTracksWithState({ getState }: IStore, action: AnyAction) { - const state = getState(); - const conference = getCurrentConference(state); - let promise; - - if (conference) { - const track = action.track.jitsiTrack; - - if (action.type === TRACK_ADDED) { - // If gUM is slow and tracks are created after the user has already joined the conference, avoid - // adding the tracks to the conference if the user is a visitor. - if (!iAmVisitor(state)) { - promise = _addLocalTracksToConference(conference, [ track ]); - } - } else { - promise = _removeLocalTracksFromConference(conference, [ track ]); - } - } - - return promise || Promise.resolve(); -} - /** * Notifies the feature base/conference that the action {@code TRACK_ADDED} * or {@code TRACK_REMOVED} is being dispatched within a specific redux store. @@ -664,9 +633,28 @@ function _trackAddedOrRemoved(store: IStore, next: Function, action: AnyAction) // TODO All track swapping should happen here instead of conference.js. if (track?.local) { - return ( - _syncConferenceLocalTracksWithState(store, action) - .then(() => next(action))); + const { getState } = store; + const state = getState(); + const conference = getCurrentConference(state); + let promise; + + if (conference) { + const jitsiTrack = action.track.jitsiTrack; + + if (action.type === TRACK_ADDED) { + // If gUM is slow and tracks are created after the user has already joined the conference, avoid + // adding the tracks to the conference if the user is a visitor. + if (!iAmVisitor(state)) { + promise = _addLocalTracksToConference(conference, [ jitsiTrack ]); + } + } else { + promise = _removeLocalTracksFromConference(conference, [ jitsiTrack ]); + } + + if (promise) { + return promise.then(() => next(action)); + } + } } return next(action); diff --git a/react/features/base/conference/middleware.web.ts b/react/features/base/conference/middleware.web.ts index bb3572dc34..3fd90623af 100644 --- a/react/features/base/conference/middleware.web.ts +++ b/react/features/base/conference/middleware.web.ts @@ -149,12 +149,15 @@ MiddlewareRegistry.register(store => next => action => { break; } case CONNECTION_ESTABLISHED: { - if (isPrejoinPageVisible(getState())) { - let { initialGUMPromise } = getState()['features/base/media']; + const { initialGUMPromise } = getState()['features/base/media']; + const promise = initialGUMPromise ? initialGUMPromise.promise : Promise.resolve({ tracks: [] }); + const prejoinVisible = isPrejoinPageVisible(getState()); - initialGUMPromise = initialGUMPromise || Promise.resolve({ tracks: [] }); + logger.debug(`On connection established: prejoinVisible: ${prejoinVisible}, initialGUMPromiseExists=${ + Boolean(initialGUMPromise)}, promiseExists=${Boolean(promise)}`); - initialGUMPromise.then(() => { + if (prejoinVisible) { + promise.then(() => { const state = getState(); let localTracks = getLocalTracks(state['features/base/tracks']); const trackReplacePromises = []; @@ -186,11 +189,7 @@ MiddlewareRegistry.register(store => next => action => { }); }); } else { - let { initialGUMPromise } = getState()['features/base/media']; - - initialGUMPromise = initialGUMPromise || Promise.resolve({ tracks: [] }); - - initialGUMPromise.then(({ tracks }) => { + promise.then(({ tracks }) => { let tracksToUse = tracks ?? []; if (iAmVisitor(getState())) { diff --git a/react/features/base/media/reducer.ts b/react/features/base/media/reducer.ts index d8a149a2f5..6f940a768f 100644 --- a/react/features/base/media/reducer.ts +++ b/react/features/base/media/reducer.ts @@ -3,6 +3,7 @@ import { AnyAction, combineReducers } from 'redux'; import { CONFERENCE_FAILED, CONFERENCE_LEFT } from '../conference/actionTypes'; import ReducerRegistry from '../redux/ReducerRegistry'; import { TRACK_REMOVED } from '../tracks/actionTypes'; +import { DefferedPromise, createDeferred } from '../util/helpers'; import { GUM_PENDING, @@ -88,6 +89,12 @@ function _audio(state: IAudioState = _AUDIO_INITIAL_MEDIA_STATE, action: AnyActi } } +// Using a deferred promise here to make sure that once the connection is established even if conference.init and the +// initial track creation haven't been started we would wait for it to finish before starting to join the room. +// NOTE: The previous implementation was using the GUM promise from conference.init. But it turned out that connect +// may finish even before conference.init is executed. +const DEFAULT_INITIAL_PROMISE_STATE = createDeferred(); + /** * Reducer fot the common properties in media state. * @@ -96,7 +103,8 @@ function _audio(state: IAudioState = _AUDIO_INITIAL_MEDIA_STATE, action: AnyActi * @param {string} action.type - Type of action. * @returns {ICommonState} */ -function _initialGUMPromise(state: initialGUMPromise | null = null, action: AnyAction) { +function _initialGUMPromise(state: DefferedPromise | null = DEFAULT_INITIAL_PROMISE_STATE, + action: AnyAction) { if (action.type === SET_INITIAL_GUM_PROMISE) { return action.promise ?? null; } @@ -264,10 +272,10 @@ interface IAudioState { unmuteBlocked: boolean; } -type initialGUMPromise = Promise<{ - errors?: any; - tracks: Array; - }> | null; +interface IInitialGUMPromiseResult { + errors?: any; + tracks: Array; +} interface IScreenshareState { available: boolean; @@ -286,7 +294,7 @@ interface IVideoState { export interface IMediaState { audio: IAudioState; - initialGUMPromise: initialGUMPromise; + initialGUMPromise: DefferedPromise | null; screenshare: IScreenshareState; video: IVideoState; } diff --git a/react/features/base/util/helpers.ts b/react/features/base/util/helpers.ts index b69362f2c3..da495f3499 100644 --- a/react/features/base/util/helpers.ts +++ b/react/features/base/util/helpers.ts @@ -22,16 +22,21 @@ export function assignIfDefined(target: Object, source: Object) { return to; } +export type DefferedPromise = { + promise: Promise; + reject: (reason?: any) => void; + resolve: (value: T) => void; +}; /** * Creates a deferred object. * * @returns {{promise, resolve, reject}} */ -export function createDeferred() { - const deferred: any = {}; +export function createDeferred() { + const deferred = {} as DefferedPromise; - deferred.promise = new Promise((resolve, reject) => { + deferred.promise = new Promise((resolve, reject) => { deferred.resolve = resolve; deferred.reject = reject; }); diff --git a/react/features/conference/actions.web.ts b/react/features/conference/actions.web.ts index c095774970..4495ede5a0 100644 --- a/react/features/conference/actions.web.ts +++ b/react/features/conference/actions.web.ts @@ -49,9 +49,11 @@ export function setupInitialDevices() { /** * Init. * + * @param {boolean} shouldDispatchConnect - Whether or not connect should be dispatched. This should be false only when + * prejoin is enabled. * @returns {Promise} */ -export function init() { +export function init(shouldDispatchConnect: boolean) { return (dispatch: IStore['dispatch'], getState: IStore['getState']) => { const room = getBackendSafeRoomName(getState()['features/base/conference'].room); @@ -59,7 +61,8 @@ export function init() { // from the old app (at the moment of writing). return dispatch(setupInitialDevices()).then( () => APP.conference.init({ - roomName: room + roomName: room, + shouldDispatchConnect }).catch((error: Error) => { APP.API.notifyConferenceLeft(APP.conference.roomName); logger.error(error); diff --git a/react/features/conference/components/web/Conference.tsx b/react/features/conference/components/web/Conference.tsx index da5abb683c..14990cb3c1 100644 --- a/react/features/conference/components/web/Conference.tsx +++ b/react/features/conference/components/web/Conference.tsx @@ -104,12 +104,24 @@ interface IProps extends AbstractProps, WithTranslation { /** * If visitors queue page is visible or not. + * NOTE: This should be set to true once we received an error on connect. Before the first connect this will always + * be false. */ _showVisitorsQueue: boolean; dispatch: IStore['dispatch']; } +/** + * Returns true if the prejoin screen should be displayed and false otherwise. + * + * @param {IProps} props - The props object. + * @returns {boolean} - True if the prejoin screen should be displayed and false otherwise. + */ +function shouldShowPrejoin({ _showPrejoin, _showVisitorsQueue }: IProps) { + return _showPrejoin && !_showVisitorsQueue; +} + /** * The conference page of the Web application. */ @@ -265,7 +277,7 @@ class Conference extends AbstractConference { - { (_showPrejoin && !_showVisitorsQueue) && } + { shouldShowPrejoin(this.props) && } { (_showLobby && !_showVisitorsQueue) && } { _showVisitorsQueue && } @@ -384,7 +396,9 @@ class Conference extends AbstractConference { const { dispatch, t } = this.props; - dispatch(init()); + // if we will be showing prejoin we don't want to call connect from init. + // Connect will be dispatched from prejoin screen. + dispatch(init(!shouldShowPrejoin(this.props))); maybeShowSuboptimalExperienceNotification(dispatch, t); }