From 99b79f62fcb629fcfe120abea73180ecfdd11829 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 10:32:59 -0600 Subject: [PATCH 1/9] Support lazy-loaded Auth --- packages/component/src/provider.ts | 40 ++++- packages/component/src/types.ts | 5 + .../All_Tests__Emulator_.xml | 2 +- ...l_Tests__Emulator_w__Mock_Persistence_.xml | 2 +- .../Integration_Tests__Emulator_.xml | 2 +- ...n_Tests__Emulator_w__Mock_Persistence_.xml | 2 +- .../.idea/runConfigurations/Unit_Tests.xml | 4 +- .../Unit_Tests__w__Mock_Persistence_.xml | 2 +- packages/firestore/src/api/credentials.ts | 158 +++++++++++++----- .../firestore/src/core/firestore_client.ts | 36 ++-- .../test/integration/util/internal_helpers.ts | 12 +- 11 files changed, 187 insertions(+), 78 deletions(-) diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index e591267a032..2a6ef0f3879 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -22,7 +22,8 @@ import { InitializeOptions, InstantiationMode, Name, - NameServiceMapping + NameServiceMapping, + OnInitCallBack } from './types'; import { Component } from './component'; @@ -37,6 +38,7 @@ export class Provider { string, Deferred > = new Map(); + private onInitCallbacks: Set> = new Set(); constructor( private readonly name: T, @@ -250,9 +252,45 @@ export class Provider { instanceDeferred.resolve(instance); } } + + this.invokeOnInitCallbacks(instance, normalizedIdentifier); return instance; } + /** + * + * @param callback - a function that will be invoked after the provider has + * been initialized by calling provider.initialize(). + * The function is invoked SYNCHRONOUSLY, so it should not execute any + * longrunning tasks in order to not block the program. + * + * @returns a function to unregister the callback + */ + onInit(callback: (instance: NameServiceMapping[T]) => void): () => void { + this.onInitCallbacks.add(callback); + + return () => { + this.onInitCallbacks.delete(callback); + }; + } + + /** + * Invoke onInit callbacks synchronously + * @param instance the service instance` + */ + private invokeOnInitCallbacks( + instance: NameServiceMapping[T], + identifier: string + ): void { + for (const callback of this.onInitCallbacks) { + try { + callback(instance, identifier); + } catch { + // ignore errors in the onInit callback + } + } + } + private getOrInitializeService({ instanceIdentifier, options = {} diff --git a/packages/component/src/types.ts b/packages/component/src/types.ts index eb5c26fb6da..fee0692314c 100644 --- a/packages/component/src/types.ts +++ b/packages/component/src/types.ts @@ -75,3 +75,8 @@ export interface NameServiceMapping {} export type Name = keyof NameServiceMapping; export type Service = NameServiceMapping[Name]; + +export type OnInitCallBack = ( + instance: NameServiceMapping[T], + identifier: string +) => void; diff --git a/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_.xml b/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_.xml index eb6ce40bb06..92d7cc57670 100644 --- a/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_.xml +++ b/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_.xml @@ -13,7 +13,7 @@ bdd --require ts-node/register/type-check --require index.node.ts --timeout 5000 PATTERN - test/{,!(browser)/**/}*.test.ts + test/{,!(browser|lite)/**/}*.test.ts \ No newline at end of file diff --git a/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_w__Mock_Persistence_.xml b/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_w__Mock_Persistence_.xml index 4e37c1bea2b..c2181e91517 100644 --- a/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_w__Mock_Persistence_.xml +++ b/packages/firestore/.idea/runConfigurations/All_Tests__Emulator_w__Mock_Persistence_.xml @@ -14,7 +14,7 @@ bdd --require ts-node/register/type-check --require index.node.ts --require test/util/node_persistence.ts --timeout 5000 PATTERN - test/{,!(browser)/**/}*.test.ts + test/{,!(browser|lite)/**/}*.test.ts \ No newline at end of file diff --git a/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_.xml b/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_.xml index be91ad5e0d3..1ea460ca6e5 100644 --- a/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_.xml +++ b/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_.xml @@ -13,7 +13,7 @@ bdd --require ts-node/register/type-check --require index.node.ts --timeout 5000 PATTERN - test/integration/{,!(browser)/**/}*.test.ts + test/integration/{,!(browser|lite)/**/}*.test.ts \ No newline at end of file diff --git a/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_w__Mock_Persistence_.xml b/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_w__Mock_Persistence_.xml index 4d0ea9ef9b0..6d9215345e1 100644 --- a/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_w__Mock_Persistence_.xml +++ b/packages/firestore/.idea/runConfigurations/Integration_Tests__Emulator_w__Mock_Persistence_.xml @@ -14,7 +14,7 @@ bdd --require ts-node/register/type-check --require index.node.ts --require test/util/node_persistence.ts --timeout 5000 PATTERN - test/integration/{,!(browser)/**/}*.test.ts + test/integration/{,!(browser|lite)/**/}*.test.ts \ No newline at end of file diff --git a/packages/firestore/.idea/runConfigurations/Unit_Tests.xml b/packages/firestore/.idea/runConfigurations/Unit_Tests.xml index a3388476f6c..5d0dcda754a 100644 --- a/packages/firestore/.idea/runConfigurations/Unit_Tests.xml +++ b/packages/firestore/.idea/runConfigurations/Unit_Tests.xml @@ -11,7 +11,7 @@ bdd --require ts-node/register/type-check --require index.node.ts PATTERN - test/unit/{,!(browser)/**/}*.test.ts + test/unit/{,!(browser|lite)/**/}*.test.ts - + \ No newline at end of file diff --git a/packages/firestore/.idea/runConfigurations/Unit_Tests__w__Mock_Persistence_.xml b/packages/firestore/.idea/runConfigurations/Unit_Tests__w__Mock_Persistence_.xml index de117facf4b..f62d34dcd93 100644 --- a/packages/firestore/.idea/runConfigurations/Unit_Tests__w__Mock_Persistence_.xml +++ b/packages/firestore/.idea/runConfigurations/Unit_Tests__w__Mock_Persistence_.xml @@ -12,7 +12,7 @@ bdd --require ts-node/register/type-check --require index.node.ts --require test/util/node_persistence.ts PATTERN - test/unit/{,!(browser)/**/}*.test.ts + test/unit/{,!(browser|lite)/**/}*.test.ts \ No newline at end of file diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 69d483a8f1d..862067452ca 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -23,8 +23,10 @@ import { Provider } from '@firebase/component'; import { User } from '../auth/user'; import { hardAssert, debugAssert } from '../util/assert'; +import { AsyncQueue } from '../util/async_queue'; import { Code, FirestoreError } from '../util/error'; import { logDebug } from '../util/log'; +import { Deferred } from '../util/promise'; // TODO(mikelehen): This should be split into multiple files and probably // moved to an auth/ folder to match other platforms. @@ -78,7 +80,7 @@ export class OAuthToken implements Token { * token and may need to invalidate other state if the current user has also * changed. */ -export type CredentialChangeListener = (user: User) => void; +export type CredentialChangeListener = (user: User) => Promise; /** * Provides methods for getting the uid and token for the current user and @@ -98,8 +100,13 @@ export interface CredentialsProvider { * Specifies a listener to be notified of credential changes * (sign-in / sign-out, token changes). It is immediately called once with the * initial user. + * + * The change listener is invoked on the provided AsyncQueue. */ - setChangeListener(changeListener: CredentialChangeListener): void; + setChangeListener( + asyncQueue: AsyncQueue, + changeListener: CredentialChangeListener + ): void; /** Removes the previously-set change listener. */ removeChangeListener(): void; @@ -120,14 +127,20 @@ export class EmptyCredentialsProvider implements CredentialsProvider { invalidateToken(): void {} - setChangeListener(changeListener: CredentialChangeListener): void { + setChangeListener( + asyncQueue: AsyncQueue, + changeListener: CredentialChangeListener + ): void { debugAssert( !this.changeListener, 'Can only call setChangeListener() once.' ); this.changeListener = changeListener; // Fire with initial user. - changeListener(User.UNAUTHENTICATED); + asyncQueue.enqueueRetryable(() => { + changeListener(User.FIRST_PARTY); + return Promise.resolve(); + }); } removeChangeListener(): void { @@ -175,11 +188,25 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * The auth token listener registered with FirebaseApp, retained here so we * can unregister it. */ - private tokenListener: ((token: string | null) => void) | null = null; + private tokenListener: (() => void) | null = null; /** Tracks the current User. */ private currentUser: User = User.UNAUTHENTICATED; - private receivedInitialUser: boolean = false; + + /** + * Promise that allows blocking on the next `tokenChange` event. The Promise + * is re-assgined in `awaitTokenAndRaiseInitialEvent()` to allow blocking on + * an a lazily loaded Auth instance. In this case, `this.receivedUser` + * resolves once when the SDK first detects that there is no synchronous + * Auth, and then gets re-created and resolves again once Auth is loaded. + */ + private receivedUser = new Deferred(); + + /** + * Whether the initial token event has been raised. This can go back to + * `false` if Firestore first starts without Auth and Auth is loaded later. + */ + private initialEventRaised: boolean = false; /** * Counter used to detect if the token changed while a getToken request was @@ -188,44 +215,62 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { private tokenCounter = 0; /** The listener registered with setChangeListener(). */ - private changeListener: CredentialChangeListener | null = null; + private changeListener: CredentialChangeListener = () => Promise.resolve(); private forceRefresh = false; - private auth: FirebaseAuthInternal | null; + private auth: FirebaseAuthInternal | null = null; + + private asyncQueue: AsyncQueue | null = null; constructor(authProvider: Provider) { this.tokenListener = () => { this.tokenCounter++; + this.receivedUser.resolve(); this.currentUser = this.getUser(); - this.receivedInitialUser = true; - if (this.changeListener) { - this.changeListener(this.currentUser); + if (this.initialEventRaised && this.asyncQueue) { + // We only invoke the change listener here if the initial event has been + // raised. The initial event itself is invoked synchronously in + // `awaitTokenAndRaiseInitialEvent()`. + this.asyncQueue.enqueueRetryable(() => { + this.changeListener(this.currentUser); + return Promise.resolve(); + }); } }; this.tokenCounter = 0; - this.auth = authProvider.getImmediate({ optional: true }); + const registerAuth = (auth: FirebaseAuthInternal): void => { + logDebug('FirebaseCredentialsProvider', 'Auth detected'); + this.auth = auth; + if (this.tokenListener) { + // tokenListener can be removed by removeChangeListener() + this.awaitTokenAndRaiseInitialEvent(); + this.auth.addAuthTokenListener(this.tokenListener); + } + }; - if (this.auth) { - this.auth.addAuthTokenListener(this.tokenListener!); - } else { - // if auth is not available, invoke tokenListener once with null token - this.tokenListener(null); - authProvider.get().then( - auth => { - this.auth = auth; - if (this.tokenListener) { - // tokenListener can be removed by removeChangeListener() - this.auth.addAuthTokenListener(this.tokenListener); - } - }, - () => { - /* this.authProvider.get() never rejects */ + authProvider.onInit(auth => registerAuth(auth)); + + // Our users can initialize Auth right after Firestore, so we give it + // a chance to register itself with the component framework before we + // determine whether to start up in unauthenticated mode. + setTimeout(() => { + if (!this.auth) { + const auth = authProvider.getImmediate({ optional: true }); + if (auth) { + registerAuth(auth); + } else if (this.tokenListener) { + // If auth is still not available, invoke tokenListener once with null + // token + logDebug('FirebaseCredentialsProvider', 'Auth not yet detected'); + this.tokenListener(); } - ); - } + } + }, 0); + + this.awaitTokenAndRaiseInitialEvent(); } getToken(): Promise { @@ -273,17 +318,13 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { this.forceRefresh = true; } - setChangeListener(changeListener: CredentialChangeListener): void { - debugAssert( - !this.changeListener, - 'Can only call setChangeListener() once.' - ); + setChangeListener( + asyncQueue: AsyncQueue, + changeListener: CredentialChangeListener + ): void { + debugAssert(!this.asyncQueue, 'Can only call setChangeListener() once.'); + this.asyncQueue = asyncQueue; this.changeListener = changeListener; - - // Fire the initial event - if (this.receivedInitialUser) { - changeListener(this.currentUser); - } } removeChangeListener(): void { @@ -291,7 +332,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { this.auth.removeAuthTokenListener(this.tokenListener!); } this.tokenListener = null; - this.changeListener = null; + this.changeListener = () => Promise.resolve(); } // Auth.getUid() can return null even with a user logged in. It is because @@ -306,6 +347,33 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { ); return new User(currentUid); } + + /** + * Blocks the AsyncQueue until the next user is available. This is invoked + * on SDK start to wait for the first user token (or `null` if Auth is not yet + * loaded). If Auth is loaded after Firestore, + * `awaitTokenAndRaiseInitialEvent()` is also used to block Firestore until + * Auth is fully initialized. + * + * This function also invokes the change listener synchronously once a token + * is available. + */ + private awaitTokenAndRaiseInitialEvent(): void { + this.initialEventRaised = false; + if (this.asyncQueue) { + // Create a new deferred Promise that gets resolved when we receive the + // next token. Ensure that all previous Promises also get resolved. + const awaitToken = new Deferred(); + void awaitToken.promise.then(() => awaitToken.resolve()); + this.receivedUser = awaitToken; + + this.asyncQueue.enqueueRetryable(async () => { + await awaitToken.promise; + await this.changeListener(this.currentUser); + }); + } + this.initialEventRaised = true; + } } // Manual type definition for the subset of Gapi we use. @@ -368,9 +436,15 @@ export class FirstPartyCredentialsProvider implements CredentialsProvider { ); } - setChangeListener(changeListener: CredentialChangeListener): void { + setChangeListener( + asyncQueue: AsyncQueue, + changeListener: CredentialChangeListener + ): void { // Fire with initial uid. - changeListener(User.FIRST_PARTY); + asyncQueue.enqueueRetryable(() => { + changeListener(User.FIRST_PARTY); + return Promise.resolve(); + }); } removeChangeListener(): void {} diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index f80b093bce0..575c2ceb5a0 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -97,8 +97,8 @@ export const MAX_CONCURRENT_LIMBO_RESOLUTIONS = 100; export class FirestoreClient { private user = User.UNAUTHENTICATED; private readonly clientId = AutoId.newId(); - private credentialListener: CredentialChangeListener = () => {}; - private readonly receivedInitialUser = new Deferred(); + private credentialListener: CredentialChangeListener = () => + Promise.resolve(); offlineComponents?: OfflineComponentProvider; onlineComponents?: OnlineComponentProvider; @@ -116,17 +116,14 @@ export class FirestoreClient { public asyncQueue: AsyncQueue, private databaseInfo: DatabaseInfo ) { - this.credentials.setChangeListener(user => { + this.credentials.setChangeListener(asyncQueue, async user => { logDebug(LOG_TAG, 'Received user=', user.uid); + await this.credentialListener(user); this.user = user; - this.credentialListener(user); - this.receivedInitialUser.resolve(); }); } async getConfiguration(): Promise { - await this.receivedInitialUser.promise; - return { asyncQueue: this.asyncQueue, databaseInfo: this.databaseInfo, @@ -137,12 +134,8 @@ export class FirestoreClient { }; } - setCredentialChangeListener(listener: (user: User) => void): void { + setCredentialChangeListener(listener: (user: User) => Promise): void { this.credentialListener = listener; - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.receivedInitialUser.promise.then(() => - this.credentialListener(this.user) - ); } /** @@ -198,15 +191,13 @@ export async function setOfflineComponentProvider( await offlineComponentProvider.initialize(configuration); let currentUser = configuration.initialUser; - client.setCredentialChangeListener(user => { + client.setCredentialChangeListener(async user => { if (!currentUser.isEqual(user)) { + await localStoreHandleUserChange( + offlineComponentProvider.localStore, + user + ); currentUser = user; - client.asyncQueue.enqueueRetryable(async () => { - await localStoreHandleUserChange( - offlineComponentProvider.localStore, - user - ); - }); } }); @@ -236,12 +227,7 @@ export async function setOnlineComponentProvider( // The CredentialChangeListener of the online component provider takes // precedence over the offline component provider. client.setCredentialChangeListener(user => - client.asyncQueue.enqueueRetryable(() => - remoteStoreHandleCredentialChange( - onlineComponentProvider.remoteStore, - user - ) - ) + remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); client.onlineComponents = onlineComponentProvider; } diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index 181b2a41bc7..4ac848d02b5 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -28,6 +28,7 @@ import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import { newConnection } from '../../../src/platform/connection'; import { newSerializer } from '../../../src/platform/serializer'; import { newDatastore, Datastore } from '../../../src/remote/datastore'; +import { AsyncQueue } from '../../../src/util/async_queue'; import { AsyncQueueImpl } from '../../../src/util/async_queue_impl'; import { TestBundleBuilder } from '../../unit/util/bundle_data'; import { collectionReference } from '../../util/api_helpers'; @@ -65,13 +66,18 @@ export function withTestDatastore( export class MockCredentialsProvider extends EmptyCredentialsProvider { private listener: CredentialChangeListener | null = null; + private asyncQueue: AsyncQueue | null = null; triggerUserChange(newUser: User): void { - this.listener!(newUser); + this.asyncQueue!.enqueueRetryable(async () => this.listener!(newUser)); } - setChangeListener(listener: CredentialChangeListener): void { - super.setChangeListener(listener); + setChangeListener( + asyncQueue: AsyncQueue, + listener: CredentialChangeListener + ): void { + super.setChangeListener(asyncQueue, listener); + this.asyncQueue = asyncQueue; this.listener = listener; } } From 1ff8800accde5b8c5f72fc7dcb3906abfb6fbd5b Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 14:14:11 -0600 Subject: [PATCH 2/9] Update packages/firestore/src/api/credentials.ts Co-authored-by: Feiyang --- packages/firestore/src/api/credentials.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 862067452ca..d8d23fae945 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -198,7 +198,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * is re-assgined in `awaitTokenAndRaiseInitialEvent()` to allow blocking on * an a lazily loaded Auth instance. In this case, `this.receivedUser` * resolves once when the SDK first detects that there is no synchronous - * Auth, and then gets re-created and resolves again once Auth is loaded. + * Auth initialization, and then gets re-created and resolves again once Auth is initialized. */ private receivedUser = new Deferred(); From c15ae9f2892091f752b2f7f6e14184f5debf7f2a Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 14:14:18 -0600 Subject: [PATCH 3/9] Update packages/firestore/src/api/credentials.ts Co-authored-by: Feiyang --- packages/firestore/src/api/credentials.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index d8d23fae945..fb4f91aefb8 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -204,7 +204,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { /** * Whether the initial token event has been raised. This can go back to - * `false` if Firestore first starts without Auth and Auth is loaded later. + * `false` if Firestore first starts without Auth and Auth is initialized later. */ private initialEventRaised: boolean = false; From 41b5dc5c4c9f7121e4caaa11be17487b8ada5c9a Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 15:48:23 -0600 Subject: [PATCH 4/9] Review --- packages/firestore/src/api/credentials.ts | 56 +++++++++-------------- 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index fb4f91aefb8..fdaf738b16a 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -137,10 +137,7 @@ export class EmptyCredentialsProvider implements CredentialsProvider { ); this.changeListener = changeListener; // Fire with initial user. - asyncQueue.enqueueRetryable(() => { - changeListener(User.FIRST_PARTY); - return Promise.resolve(); - }); + asyncQueue.enqueueRetryable(() => changeListener(User.FIRST_PARTY)); } removeChangeListener(): void { @@ -168,14 +165,17 @@ export class EmulatorCredentialsProvider implements CredentialsProvider { invalidateToken(): void {} - setChangeListener(changeListener: CredentialChangeListener): void { + setChangeListener( + asyncQueue: AsyncQueue, + changeListener: CredentialChangeListener + ): void { debugAssert( !this.changeListener, 'Can only call setChangeListener() once.' ); this.changeListener = changeListener; // Fire with initial user. - changeListener(this.token.user); + asyncQueue.enqueueRetryable(() => changeListener(this.token.user)); } removeChangeListener(): void { @@ -188,26 +188,21 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * The auth token listener registered with FirebaseApp, retained here so we * can unregister it. */ - private tokenListener: (() => void) | null = null; + private tokenListener: () => void; /** Tracks the current User. */ private currentUser: User = User.UNAUTHENTICATED; /** * Promise that allows blocking on the next `tokenChange` event. The Promise - * is re-assgined in `awaitTokenAndRaiseInitialEvent()` to allow blocking on + * is reassigned in `awaitTokenAndRaiseInitialEvent()` to allow blocking on * an a lazily loaded Auth instance. In this case, `this.receivedUser` * resolves once when the SDK first detects that there is no synchronous - * Auth initialization, and then gets re-created and resolves again once Auth is initialized. + * Auth initialization, and then gets re-created and resolves again once Auth + * is initialized. */ private receivedUser = new Deferred(); - /** - * Whether the initial token event has been raised. This can go back to - * `false` if Firestore first starts without Auth and Auth is initialized later. - */ - private initialEventRaised: boolean = false; - /** * Counter used to detect if the token changed while a getToken request was * outstanding. @@ -217,6 +212,8 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { /** The listener registered with setChangeListener(). */ private changeListener: CredentialChangeListener = () => Promise.resolve(); + private invokeChangeListener = false; + private forceRefresh = false; private auth: FirebaseAuthInternal | null = null; @@ -228,14 +225,10 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { this.tokenCounter++; this.receivedUser.resolve(); this.currentUser = this.getUser(); - if (this.initialEventRaised && this.asyncQueue) { - // We only invoke the change listener here if the initial event has been - // raised. The initial event itself is invoked synchronously in - // `awaitTokenAndRaiseInitialEvent()`. - this.asyncQueue.enqueueRetryable(() => { - this.changeListener(this.currentUser); - return Promise.resolve(); - }); + if (this.invokeChangeListener) { + this.asyncQueue!.enqueueRetryable(() => + this.changeListener(this.currentUser) + ); } }; @@ -244,11 +237,8 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { const registerAuth = (auth: FirebaseAuthInternal): void => { logDebug('FirebaseCredentialsProvider', 'Auth detected'); this.auth = auth; - if (this.tokenListener) { - // tokenListener can be removed by removeChangeListener() - this.awaitTokenAndRaiseInitialEvent(); - this.auth.addAuthTokenListener(this.tokenListener); - } + this.awaitTokenAndRaiseInitialEvent(); + this.auth.addAuthTokenListener(this.tokenListener); }; authProvider.onInit(auth => registerAuth(auth)); @@ -331,7 +321,6 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { if (this.auth) { this.auth.removeAuthTokenListener(this.tokenListener!); } - this.tokenListener = null; this.changeListener = () => Promise.resolve(); } @@ -359,7 +348,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * is available. */ private awaitTokenAndRaiseInitialEvent(): void { - this.initialEventRaised = false; + this.invokeChangeListener = false; // Prevent double-firing of the listener if (this.asyncQueue) { // Create a new deferred Promise that gets resolved when we receive the // next token. Ensure that all previous Promises also get resolved. @@ -370,9 +359,9 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { this.asyncQueue.enqueueRetryable(async () => { await awaitToken.promise; await this.changeListener(this.currentUser); + this.invokeChangeListener = true; }); } - this.initialEventRaised = true; } } @@ -441,10 +430,7 @@ export class FirstPartyCredentialsProvider implements CredentialsProvider { changeListener: CredentialChangeListener ): void { // Fire with initial uid. - asyncQueue.enqueueRetryable(() => { - changeListener(User.FIRST_PARTY); - return Promise.resolve(); - }); + asyncQueue.enqueueRetryable(() => changeListener(User.FIRST_PARTY)); } removeChangeListener(): void {} From 51718f1dce73cceac93becf556f705db1eaf9ece Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 18:15:20 -0600 Subject: [PATCH 5/9] Simplify --- packages/firestore/src/api/credentials.ts | 27 +++++++++-------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index fdaf738b16a..0505c8b92b7 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -232,8 +232,6 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { } }; - this.tokenCounter = 0; - const registerAuth = (auth: FirebaseAuthInternal): void => { logDebug('FirebaseCredentialsProvider', 'Auth detected'); this.auth = auth; @@ -251,16 +249,16 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { const auth = authProvider.getImmediate({ optional: true }); if (auth) { registerAuth(auth); - } else if (this.tokenListener) { + } else if (this.invokeChangeListener) { // If auth is still not available, invoke tokenListener once with null // token logDebug('FirebaseCredentialsProvider', 'Auth not yet detected'); - this.tokenListener(); + this.asyncQueue!.enqueueRetryable(() => + this.changeListener(this.currentUser) + ); } } }, 0); - - this.awaitTokenAndRaiseInitialEvent(); } getToken(): Promise { @@ -313,6 +311,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { changeListener: CredentialChangeListener ): void { debugAssert(!this.asyncQueue, 'Can only call setChangeListener() once.'); + this.invokeChangeListener = true; this.asyncQueue = asyncQueue; this.changeListener = changeListener; } @@ -344,20 +343,14 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * `awaitTokenAndRaiseInitialEvent()` is also used to block Firestore until * Auth is fully initialized. * - * This function also invokes the change listener synchronously once a token + * This function also invokes the change listener immediately after the token * is available. */ private awaitTokenAndRaiseInitialEvent(): void { - this.invokeChangeListener = false; // Prevent double-firing of the listener - if (this.asyncQueue) { - // Create a new deferred Promise that gets resolved when we receive the - // next token. Ensure that all previous Promises also get resolved. - const awaitToken = new Deferred(); - void awaitToken.promise.then(() => awaitToken.resolve()); - this.receivedUser = awaitToken; - - this.asyncQueue.enqueueRetryable(async () => { - await awaitToken.promise; + if (this.invokeChangeListener) { + this.invokeChangeListener = false; // Prevent double-firing of the listener + this.asyncQueue!.enqueueRetryable(async () => { + await this.receivedUser.promise; await this.changeListener(this.currentUser); this.invokeChangeListener = true; }); From 3624380d07ca5b30831a23b4d875943809407338 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 19:24:28 -0600 Subject: [PATCH 6/9] Update provider.ts --- packages/component/src/provider.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/component/src/provider.ts b/packages/component/src/provider.ts index 809c16a791b..d5a171b6c3a 100644 --- a/packages/component/src/provider.ts +++ b/packages/component/src/provider.ts @@ -255,7 +255,6 @@ export class Provider { this.invokeOnInitCallbacks(instance, normalizedIdentifier); - return instance; } From 0faa3d977fcb9d3149f4fe5c5baaf06b2800f20d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 19:26:42 -0600 Subject: [PATCH 7/9] Cleanup --- packages/firestore/src/api/credentials.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 0505c8b92b7..6430be6dda6 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -137,7 +137,7 @@ export class EmptyCredentialsProvider implements CredentialsProvider { ); this.changeListener = changeListener; // Fire with initial user. - asyncQueue.enqueueRetryable(() => changeListener(User.FIRST_PARTY)); + asyncQueue.enqueueRetryable(() => changeListener(User.UNAUTHENTICATED)); } removeChangeListener(): void { @@ -201,7 +201,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { * Auth initialization, and then gets re-created and resolves again once Auth * is initialized. */ - private receivedUser = new Deferred(); + private receivedInitialUser = new Deferred(); /** * Counter used to detect if the token changed while a getToken request was @@ -223,8 +223,8 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { constructor(authProvider: Provider) { this.tokenListener = () => { this.tokenCounter++; - this.receivedUser.resolve(); this.currentUser = this.getUser(); + this.receivedInitialUser.resolve(); if (this.invokeChangeListener) { this.asyncQueue!.enqueueRetryable(() => this.changeListener(this.currentUser) @@ -350,7 +350,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { if (this.invokeChangeListener) { this.invokeChangeListener = false; // Prevent double-firing of the listener this.asyncQueue!.enqueueRetryable(async () => { - await this.receivedUser.promise; + await this.receivedInitialUser.promise; await this.changeListener(this.currentUser); this.invokeChangeListener = true; }); From 958bcdfd90d7774f9ca0f9744de4ac537cdf6a0b Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 4 May 2021 19:35:22 -0600 Subject: [PATCH 8/9] Comment cleanup --- packages/firestore/src/api/credentials.ts | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 6430be6dda6..1054f847115 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -193,14 +193,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { /** Tracks the current User. */ private currentUser: User = User.UNAUTHENTICATED; - /** - * Promise that allows blocking on the next `tokenChange` event. The Promise - * is reassigned in `awaitTokenAndRaiseInitialEvent()` to allow blocking on - * an a lazily loaded Auth instance. In this case, `this.receivedUser` - * resolves once when the SDK first detects that there is no synchronous - * Auth initialization, and then gets re-created and resolves again once Auth - * is initialized. - */ + /** Promise that allows blocking on the first `tokenListener` event. */ private receivedInitialUser = new Deferred(); /** @@ -337,14 +330,8 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { } /** - * Blocks the AsyncQueue until the next user is available. This is invoked - * on SDK start to wait for the first user token (or `null` if Auth is not yet - * loaded). If Auth is loaded after Firestore, - * `awaitTokenAndRaiseInitialEvent()` is also used to block Firestore until - * Auth is fully initialized. - * - * This function also invokes the change listener immediately after the token - * is available. + * Blocks the AsyncQueue until the next user is available. This function also + * invokes `this.changeListener` immediately once the token is available. */ private awaitTokenAndRaiseInitialEvent(): void { if (this.invokeChangeListener) { From be1fa3c06e1032176112a0626b72ceeb36d4f9fb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 5 May 2021 09:05:46 -0600 Subject: [PATCH 9/9] Review --- packages/firestore/src/api/credentials.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 1054f847115..566e393b274 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -243,8 +243,8 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { if (auth) { registerAuth(auth); } else if (this.invokeChangeListener) { - // If auth is still not available, invoke tokenListener once with null - // token + // If auth is still not available, invoke the change listener once + // with null token logDebug('FirebaseCredentialsProvider', 'Auth not yet detected'); this.asyncQueue!.enqueueRetryable(() => this.changeListener(this.currentUser)