From 47b09132463d6a038b441d4623c24ca61e56505d Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 20 Sep 2024 15:29:50 -0400 Subject: [PATCH] Support restart of FirestoreClient (#8430) * Support restart of FirestoreClient * Fix errors * Pretty * Fix * lint * pretty * fix * undo not required changes * Only appy required test changes * Apply suggestions from Andy * Fixes from PR review --- .changeset/small-geckos-mix.md | 5 + packages/firestore/src/api/cache_config.ts | 83 +++---- packages/firestore/src/api/credentials.ts | 33 ++- packages/firestore/src/api/database.ts | 207 +++++++++--------- .../firestore/src/api/index_configuration.ts | 2 +- .../src/api/persistent_cache_index_manager.ts | 19 +- .../firestore/src/core/component_provider.ts | 22 ++ .../firestore/src/core/firestore_client.ts | 31 +-- packages/firestore/src/lite-api/database.ts | 23 +- packages/firestore/src/util/async_observer.ts | 18 +- .../firestore/src/util/async_queue_impl.ts | 5 +- .../test/integration/api/database.test.ts | 20 ++ .../test/integration/api/validation.test.ts | 8 +- 13 files changed, 274 insertions(+), 202 deletions(-) create mode 100644 .changeset/small-geckos-mix.md diff --git a/.changeset/small-geckos-mix.md b/.changeset/small-geckos-mix.md new file mode 100644 index 00000000000..33ddb4a7116 --- /dev/null +++ b/.changeset/small-geckos-mix.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Refactor Firestore client instantiation. This prepares for future features that require client to restart. diff --git a/packages/firestore/src/api/cache_config.ts b/packages/firestore/src/api/cache_config.ts index 1007023b2d7..fcfe429f554 100644 --- a/packages/firestore/src/api/cache_config.ts +++ b/packages/firestore/src/api/cache_config.ts @@ -20,7 +20,8 @@ import { LruGcMemoryOfflineComponentProvider, MemoryOfflineComponentProvider, MultiTabOfflineComponentProvider, - OfflineComponentProvider, + OfflineComponentProviderFactory, + OnlineComponentProviderFactory, OnlineComponentProvider } from '../core/component_provider'; @@ -38,11 +39,11 @@ export type MemoryLocalCache = { /** * @internal */ - _onlineComponentProvider: OnlineComponentProvider; + _onlineComponentProvider: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; }; class MemoryLocalCacheImpl implements MemoryLocalCache { @@ -50,19 +51,19 @@ class MemoryLocalCacheImpl implements MemoryLocalCache { /** * @internal */ - _onlineComponentProvider: OnlineComponentProvider; + _onlineComponentProvider: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; constructor(settings?: MemoryCacheSettings) { - this._onlineComponentProvider = new OnlineComponentProvider(); + this._onlineComponentProvider = OnlineComponentProvider.provider; if (settings?.garbageCollector) { this._offlineComponentProvider = settings.garbageCollector._offlineComponentProvider; } else { - this._offlineComponentProvider = new MemoryOfflineComponentProvider(); + this._offlineComponentProvider = MemoryOfflineComponentProvider.provider; } } @@ -83,11 +84,11 @@ export type PersistentLocalCache = { /** * @internal */ - _onlineComponentProvider: OnlineComponentProvider; + _onlineComponentProvider: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider: OfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; }; class PersistentLocalCacheImpl implements PersistentLocalCache { @@ -95,11 +96,11 @@ class PersistentLocalCacheImpl implements PersistentLocalCache { /** * @internal */ - _onlineComponentProvider: OnlineComponentProvider; + _onlineComponentProvider: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider: OfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; constructor(settings: PersistentCacheSettings | undefined) { let tabManager: PersistentTabManager; @@ -147,7 +148,7 @@ export type MemoryEagerGarbageCollector = { /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; }; /** @@ -167,7 +168,7 @@ export type MemoryLruGarbageCollector = { /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; }; class MemoryEagerGarbageCollectorImpl implements MemoryEagerGarbageCollector { @@ -175,10 +176,10 @@ class MemoryEagerGarbageCollectorImpl implements MemoryEagerGarbageCollector { /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; constructor() { - this._offlineComponentProvider = new MemoryOfflineComponentProvider(); + this._offlineComponentProvider = MemoryOfflineComponentProvider.provider; } toJSON(): {} { @@ -191,12 +192,12 @@ class MemoryLruGarbageCollectorImpl implements MemoryLruGarbageCollector { /** * @internal */ - _offlineComponentProvider: MemoryOfflineComponentProvider; + _offlineComponentProvider: OfflineComponentProviderFactory; constructor(cacheSize?: number) { - this._offlineComponentProvider = new LruGcMemoryOfflineComponentProvider( - cacheSize - ); + this._offlineComponentProvider = { + build: () => new LruGcMemoryOfflineComponentProvider(cacheSize) + }; } toJSON(): {} { @@ -297,11 +298,11 @@ export type PersistentSingleTabManager = { /** * @internal */ - _onlineComponentProvider?: OnlineComponentProvider; + _onlineComponentProvider?: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider?: OfflineComponentProvider; + _offlineComponentProvider?: OfflineComponentProviderFactory; }; class SingleTabManagerImpl implements PersistentSingleTabManager { @@ -310,11 +311,11 @@ class SingleTabManagerImpl implements PersistentSingleTabManager { /** * @internal */ - _onlineComponentProvider?: OnlineComponentProvider; + _onlineComponentProvider?: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider?: OfflineComponentProvider; + _offlineComponentProvider?: OfflineComponentProviderFactory; constructor(private forceOwnership?: boolean) {} @@ -328,12 +329,15 @@ class SingleTabManagerImpl implements PersistentSingleTabManager { _initialize( settings: Omit | undefined ): void { - this._onlineComponentProvider = new OnlineComponentProvider(); - this._offlineComponentProvider = new IndexedDbOfflineComponentProvider( - this._onlineComponentProvider, - settings?.cacheSizeBytes, - this.forceOwnership - ); + this._onlineComponentProvider = OnlineComponentProvider.provider; + this._offlineComponentProvider = { + build: (onlineComponents: OnlineComponentProvider) => + new IndexedDbOfflineComponentProvider( + onlineComponents, + settings?.cacheSizeBytes, + this.forceOwnership + ) + }; } } @@ -350,12 +354,12 @@ export type PersistentMultipleTabManager = { /** * @internal */ - _onlineComponentProvider?: OnlineComponentProvider; + _onlineComponentProvider?: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider?: OfflineComponentProvider; + _offlineComponentProvider?: OfflineComponentProviderFactory; }; class MultiTabManagerImpl implements PersistentMultipleTabManager { @@ -364,11 +368,11 @@ class MultiTabManagerImpl implements PersistentMultipleTabManager { /** * @internal */ - _onlineComponentProvider?: OnlineComponentProvider; + _onlineComponentProvider?: OnlineComponentProviderFactory; /** * @internal */ - _offlineComponentProvider?: OfflineComponentProvider; + _offlineComponentProvider?: OfflineComponentProviderFactory; toJSON(): {} { return { kind: this.kind }; @@ -380,11 +384,14 @@ class MultiTabManagerImpl implements PersistentMultipleTabManager { _initialize( settings: Omit | undefined ): void { - this._onlineComponentProvider = new OnlineComponentProvider(); - this._offlineComponentProvider = new MultiTabOfflineComponentProvider( - this._onlineComponentProvider, - settings?.cacheSizeBytes - ); + this._onlineComponentProvider = OnlineComponentProvider.provider; + this._offlineComponentProvider = { + build: (onlineComponents: OnlineComponentProvider) => + new MultiTabOfflineComponentProvider( + onlineComponents, + settings?.cacheSizeBytes + ) + }; } } diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index 662439a04bb..5da9a32209b 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -235,7 +235,7 @@ export class FirebaseAuthCredentialsProvider * The auth token listener registered with FirebaseApp, retained here so we * can unregister it. */ - private tokenListener!: () => void; + private tokenListener: (() => void) | undefined; /** Tracks the current User. */ private currentUser: User = User.UNAUTHENTICATED; @@ -256,6 +256,10 @@ export class FirebaseAuthCredentialsProvider asyncQueue: AsyncQueue, changeListener: CredentialChangeListener ): void { + hardAssert( + this.tokenListener === undefined, + 'Token listener already added' + ); let lastTokenId = this.tokenCounter; // A change listener that prevents double-firing for the same token change. @@ -293,8 +297,10 @@ export class FirebaseAuthCredentialsProvider const registerAuth = (auth: FirebaseAuthInternal): void => { logDebug('FirebaseAuthCredentialsProvider', 'Auth detected'); this.auth = auth; - this.auth.addAuthTokenListener(this.tokenListener); - awaitNextToken(); + if (this.tokenListener) { + this.auth.addAuthTokenListener(this.tokenListener); + awaitNextToken(); + } }; this.authProvider.onInit(auth => registerAuth(auth)); @@ -365,9 +371,10 @@ export class FirebaseAuthCredentialsProvider } shutdown(): void { - if (this.auth) { - this.auth.removeAuthTokenListener(this.tokenListener!); + if (this.auth && this.tokenListener) { + this.auth.removeAuthTokenListener(this.tokenListener); } + this.tokenListener = undefined; } // Auth.getUid() can return null even with a user logged in. It is because @@ -484,7 +491,7 @@ export class FirebaseAppCheckTokenProvider * The AppCheck token listener registered with FirebaseApp, retained here so * we can unregister it. */ - private tokenListener!: AppCheckTokenListener; + private tokenListener: AppCheckTokenListener | undefined; private forceRefresh = false; private appCheck: FirebaseAppCheckInternal | null = null; private latestAppCheckToken: string | null = null; @@ -497,6 +504,11 @@ export class FirebaseAppCheckTokenProvider asyncQueue: AsyncQueue, changeListener: CredentialChangeListener ): void { + hardAssert( + this.tokenListener === undefined, + 'Token listener already added' + ); + const onTokenChanged: ( tokenResult: AppCheckTokenResult ) => Promise = tokenResult => { @@ -524,7 +536,9 @@ export class FirebaseAppCheckTokenProvider const registerAppCheck = (appCheck: FirebaseAppCheckInternal): void => { logDebug('FirebaseAppCheckTokenProvider', 'AppCheck detected'); this.appCheck = appCheck; - this.appCheck.addTokenListener(this.tokenListener); + if (this.tokenListener) { + this.appCheck.addTokenListener(this.tokenListener); + } }; this.appCheckProvider.onInit(appCheck => registerAppCheck(appCheck)); @@ -579,9 +593,10 @@ export class FirebaseAppCheckTokenProvider } shutdown(): void { - if (this.appCheck) { - this.appCheck.removeTokenListener(this.tokenListener!); + if (this.appCheck && this.tokenListener) { + this.appCheck.removeTokenListener(this.tokenListener); } + this.tokenListener = undefined; } } diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index da0ef4f8eae..0757378a74c 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -28,24 +28,23 @@ import { IndexedDbOfflineComponentProvider, MultiTabOfflineComponentProvider, OfflineComponentProvider, - OnlineComponentProvider + OfflineComponentProviderFactory, + OnlineComponentProvider, + OnlineComponentProviderFactory } from '../core/component_provider'; import { DatabaseId, DEFAULT_DATABASE_NAME } from '../core/database_info'; import { - canFallbackFromIndexedDbError, FirestoreClient, firestoreClientDisableNetwork, firestoreClientEnableNetwork, firestoreClientGetNamedQuery, firestoreClientLoadBundle, - firestoreClientWaitForPendingWrites, - setOfflineComponentProvider, - setOnlineComponentProvider + firestoreClientWaitForPendingWrites } from '../core/firestore_client'; import { makeDatabaseInfo } from '../lite-api/components'; import { - Firestore as LiteFirestore, - connectFirestoreEmulator + connectFirestoreEmulator, + Firestore as LiteFirestore } from '../lite-api/database'; import { Query } from '../lite-api/reference'; import { @@ -56,7 +55,7 @@ import { LRU_COLLECTION_DISABLED } from '../local/lru_garbage_collector'; import { LRU_MINIMUM_CACHE_SIZE_BYTES } from '../local/lru_garbage_collector_impl'; import { debugAssert } from '../util/assert'; import { AsyncQueue } from '../util/async_queue'; -import { newAsyncQueue } from '../util/async_queue_impl'; +import { AsyncQueueImpl } from '../util/async_queue_impl'; import { Code, FirestoreError } from '../util/error'; import { cast } from '../util/input_validation'; import { logWarn } from '../util/log'; @@ -64,7 +63,8 @@ import { Deferred } from '../util/promise'; import { LoadBundleTask } from './bundle'; import { CredentialsProvider } from './credentials'; -import { PersistenceSettings, FirestoreSettings } from './settings'; +import { FirestoreSettings, PersistenceSettings } from './settings'; + export { connectFirestoreEmulator, EmulatorMockTokenOptions @@ -94,11 +94,16 @@ export class Firestore extends LiteFirestore { */ type: 'firestore-lite' | 'firestore' = 'firestore'; - readonly _queue: AsyncQueue = newAsyncQueue(); + _queue: AsyncQueue = new AsyncQueueImpl(); readonly _persistenceKey: string; _firestoreClient: FirestoreClient | undefined; + _componentsProvider?: { + _offline: OfflineComponentProviderFactory; + _online: OnlineComponentProviderFactory; + }; + /** @hideconstructor */ constructor( authCredentialsProvider: CredentialsProvider, @@ -115,13 +120,13 @@ export class Firestore extends LiteFirestore { this._persistenceKey = app?.name || '[DEFAULT]'; } - _terminate(): Promise { - if (!this._firestoreClient) { - // The client must be initialized to ensure that all subsequent API - // usage throws an exception. - configureFirestore(this); + protected async _terminate(): Promise { + if (this._firestoreClient) { + const terminate = this._firestoreClient.terminate(); + this._queue = new AsyncQueueImpl(terminate); + this._firestoreClient = undefined; + await terminate; } - return this._firestoreClient!.terminate(); } } @@ -263,10 +268,15 @@ export function getFirestore( export function ensureFirestoreConfigured( firestore: Firestore ): FirestoreClient { + if (firestore._terminated) { + throw new FirestoreError( + Code.FAILED_PRECONDITION, + 'The client has already been terminated.' + ); + } if (!firestore._firestoreClient) { configureFirestore(firestore); } - firestore._firestoreClient!.verifyNotTerminated(); return firestore._firestoreClient as FirestoreClient; } @@ -284,22 +294,39 @@ export function configureFirestore(firestore: Firestore): void { firestore._persistenceKey, settings ); + if (!firestore._componentsProvider) { + if ( + settings.localCache?._offlineComponentProvider && + settings.localCache?._onlineComponentProvider + ) { + firestore._componentsProvider = { + _offline: settings.localCache._offlineComponentProvider, + _online: settings.localCache._onlineComponentProvider + }; + } + } firestore._firestoreClient = new FirestoreClient( firestore._authCredentials, firestore._appCheckCredentials, firestore._queue, - databaseInfo + databaseInfo, + firestore._componentsProvider && + buildComponentProvider(firestore._componentsProvider) ); - if ( - settings.localCache?._offlineComponentProvider && - settings.localCache?._onlineComponentProvider - ) { - firestore._firestoreClient._uninitializedComponentsProvider = { - _offlineKind: settings.localCache.kind, - _offline: settings.localCache._offlineComponentProvider, - _online: settings.localCache._onlineComponentProvider - }; - } +} + +function buildComponentProvider(componentsProvider: { + _offline: OfflineComponentProviderFactory; + _online: OnlineComponentProviderFactory; +}): { + _offline: OfflineComponentProvider; + _online: OnlineComponentProvider; +} { + const online = componentsProvider?._online.build(); + return { + _offline: componentsProvider?._offline.build(online), + _online: online + }; } /** @@ -335,34 +362,21 @@ export function enableIndexedDbPersistence( firestore: Firestore, persistenceSettings?: PersistenceSettings ): Promise { - firestore = cast(firestore, Firestore); - verifyNotInitialized(firestore); - - const client = ensureFirestoreConfigured(firestore); - if (client._uninitializedComponentsProvider) { - throw new FirestoreError( - Code.FAILED_PRECONDITION, - 'SDK cache is already specified.' - ); - } - logWarn( 'enableIndexedDbPersistence() will be deprecated in the future, ' + 'you can use `FirestoreSettings.cache` instead.' ); const settings = firestore._freezeSettings(); - const onlineComponentProvider = new OnlineComponentProvider(); - const offlineComponentProvider = new IndexedDbOfflineComponentProvider( - onlineComponentProvider, - settings.cacheSizeBytes, - persistenceSettings?.forceOwnership - ); - return setPersistenceProviders( - client, - onlineComponentProvider, - offlineComponentProvider - ); + setPersistenceProviders(firestore, OnlineComponentProvider.provider, { + build: (onlineComponents: OnlineComponentProvider) => + new IndexedDbOfflineComponentProvider( + onlineComponents, + settings.cacheSizeBytes, + persistenceSettings?.forceOwnership + ) + }); + return Promise.resolve(); } /** @@ -391,36 +405,22 @@ export function enableIndexedDbPersistence( * turn on indexeddb cache. Calling this function when `FirestoreSettings.localCache` * is already specified will throw an exception. */ -export function enableMultiTabIndexedDbPersistence( +export async function enableMultiTabIndexedDbPersistence( firestore: Firestore ): Promise { - firestore = cast(firestore, Firestore); - verifyNotInitialized(firestore); - - const client = ensureFirestoreConfigured(firestore); - if (client._uninitializedComponentsProvider) { - throw new FirestoreError( - Code.FAILED_PRECONDITION, - 'SDK cache is already specified.' - ); - } - logWarn( 'enableMultiTabIndexedDbPersistence() will be deprecated in the future, ' + 'you can use `FirestoreSettings.cache` instead.' ); const settings = firestore._freezeSettings(); - const onlineComponentProvider = new OnlineComponentProvider(); - const offlineComponentProvider = new MultiTabOfflineComponentProvider( - onlineComponentProvider, - settings.cacheSizeBytes - ); - return setPersistenceProviders( - client, - onlineComponentProvider, - offlineComponentProvider - ); + setPersistenceProviders(firestore, OnlineComponentProvider.provider, { + build: (onlineComponents: OnlineComponentProvider) => + new MultiTabOfflineComponentProvider( + onlineComponents, + settings.cacheSizeBytes + ) + }); } /** @@ -430,31 +430,33 @@ export function enableMultiTabIndexedDbPersistence( * but the client remains usable. */ function setPersistenceProviders( - client: FirestoreClient, - onlineComponentProvider: OnlineComponentProvider, - offlineComponentProvider: OfflineComponentProvider -): Promise { - const persistenceResult = new Deferred(); - return client.asyncQueue - .enqueue(async () => { - try { - await setOfflineComponentProvider(client, offlineComponentProvider); - await setOnlineComponentProvider(client, onlineComponentProvider); - persistenceResult.resolve(); - } catch (e) { - const error = e as FirestoreError | DOMException; - if (!canFallbackFromIndexedDbError(error)) { - throw error; - } - logWarn( - 'Error enabling indexeddb cache. Falling back to ' + - 'memory cache: ' + - error - ); - persistenceResult.reject(error); - } - }) - .then(() => persistenceResult.promise); + firestore: Firestore, + onlineComponentProvider: OnlineComponentProviderFactory, + offlineComponentProvider: OfflineComponentProviderFactory +): void { + firestore = cast(firestore, Firestore); + if (firestore._firestoreClient || firestore._terminated) { + throw new FirestoreError( + Code.FAILED_PRECONDITION, + 'Firestore has already been started and persistence can no longer be ' + + 'enabled. You can only enable persistence before calling any other ' + + 'methods on a Firestore object.' + ); + } + + if (firestore._componentsProvider || firestore._getSettings().localCache) { + throw new FirestoreError( + Code.FAILED_PRECONDITION, + 'SDK cache is already specified.' + ); + } + + firestore._componentsProvider = { + _online: onlineComponentProvider, + _offline: offlineComponentProvider + }; + + configureFirestore(firestore); } /** @@ -634,14 +636,3 @@ export function namedQuery( return new Query(firestore, null, namedQuery.query); }); } - -function verifyNotInitialized(firestore: Firestore): void { - if (firestore._initialized || firestore._terminated) { - throw new FirestoreError( - Code.FAILED_PRECONDITION, - 'Firestore has already been started and persistence can no longer be ' + - 'enabled. You can only enable persistence before calling any other ' + - 'methods on a Firestore object.' - ); - } -} diff --git a/packages/firestore/src/api/index_configuration.ts b/packages/firestore/src/api/index_configuration.ts index d482738280b..aaa94ce3bbe 100644 --- a/packages/firestore/src/api/index_configuration.ts +++ b/packages/firestore/src/api/index_configuration.ts @@ -175,7 +175,7 @@ export function setIndexConfiguration( const client = ensureFirestoreConfigured(firestore); if ( !client._uninitializedComponentsProvider || - client._uninitializedComponentsProvider?._offlineKind === 'memory' + client._uninitializedComponentsProvider._offline.kind === 'memory' ) { // PORTING NOTE: We don't return an error if the user has not enabled // persistence since `enableIndexeddbPersistence()` can fail on the Web. diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts index 0e7f4d17614..6568ebde1d4 100644 --- a/packages/firestore/src/api/persistent_cache_index_manager.ts +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -17,8 +17,7 @@ import { firestoreClientDeleteAllFieldIndexes, - firestoreClientSetPersistentCacheIndexAutoCreationEnabled, - FirestoreClient + firestoreClientSetPersistentCacheIndexAutoCreationEnabled } from '../core/firestore_client'; import { cast } from '../util/input_validation'; import { logDebug, logWarn } from '../util/log'; @@ -36,7 +35,7 @@ export class PersistentCacheIndexManager { readonly type: 'PersistentCacheIndexManager' = 'PersistentCacheIndexManager'; /** @hideconstructor */ - constructor(readonly _client: FirestoreClient) {} + constructor(readonly _firestore: Firestore) {} } /** @@ -57,11 +56,11 @@ export function getPersistentCacheIndexManager( } const client = ensureFirestoreConfigured(firestore); - if (client._uninitializedComponentsProvider?._offlineKind !== 'persistent') { + if (client._uninitializedComponentsProvider?._offline.kind !== 'persistent') { return null; } - const instance = new PersistentCacheIndexManager(client); + const instance = new PersistentCacheIndexManager(firestore); persistentCacheIndexManagerByFirestore.set(firestore, instance); return instance; } @@ -99,9 +98,8 @@ export function disablePersistentCacheIndexAutoCreation( export function deleteAllPersistentCacheIndexes( indexManager: PersistentCacheIndexManager ): void { - indexManager._client.verifyNotTerminated(); - - const promise = firestoreClientDeleteAllFieldIndexes(indexManager._client); + const client = ensureFirestoreConfigured(indexManager._firestore); + const promise = firestoreClientDeleteAllFieldIndexes(client); promise .then(_ => logDebug('deleting all persistent cache indexes succeeded')) @@ -114,10 +112,9 @@ function setPersistentCacheIndexAutoCreationEnabled( indexManager: PersistentCacheIndexManager, isEnabled: boolean ): void { - indexManager._client.verifyNotTerminated(); - + const client = ensureFirestoreConfigured(indexManager._firestore); const promise = firestoreClientSetPersistentCacheIndexAutoCreationEnabled( - indexManager._client, + client, isEnabled ); diff --git a/packages/firestore/src/core/component_provider.ts b/packages/firestore/src/core/component_provider.ts index a3153e797b2..8a63509232c 100644 --- a/packages/firestore/src/core/component_provider.ts +++ b/packages/firestore/src/core/component_provider.ts @@ -75,6 +75,8 @@ import { } from './sync_engine_impl'; import { OnlineStateSource } from './types'; +type Kind = 'memory' | 'persistent'; + export interface ComponentConfiguration { asyncQueue: AsyncQueue; databaseInfo: DatabaseInfo; @@ -85,11 +87,16 @@ export interface ComponentConfiguration { maxConcurrentLimboResolutions: number; } +export interface OfflineComponentProviderFactory { + build(onlineComponents: OnlineComponentProvider): OfflineComponentProvider; +} + /** * Initializes and wires components that are needed to interface with the local * cache. Implementations override `initialize()` to provide all components. */ export interface OfflineComponentProvider { + readonly kind: Kind; persistence: Persistence; sharedClientState: SharedClientState; localStore: LocalStore; @@ -109,6 +116,12 @@ export interface OfflineComponentProvider { export class MemoryOfflineComponentProvider implements OfflineComponentProvider { + kind: Kind = 'memory'; + + static readonly provider: OfflineComponentProviderFactory = { + build: () => new MemoryOfflineComponentProvider() + }; + persistence!: Persistence; sharedClientState!: SharedClientState; localStore!: LocalStore; @@ -208,6 +221,7 @@ export class LruGcMemoryOfflineComponentProvider extends MemoryOfflineComponentP * Provides all components needed for Firestore with IndexedDB persistence. */ export class IndexedDbOfflineComponentProvider extends MemoryOfflineComponentProvider { + kind: Kind = 'persistent'; persistence!: IndexedDbPersistence; sharedClientState!: SharedClientState; localStore!: LocalStore; @@ -389,11 +403,19 @@ export class MultiTabOfflineComponentProvider extends IndexedDbOfflineComponentP } } +export interface OnlineComponentProviderFactory { + build(): OnlineComponentProvider; +} + /** * Initializes and wires the components that are needed to interface with the * network. */ export class OnlineComponentProvider { + static readonly provider: OnlineComponentProviderFactory = { + build: () => new OnlineComponentProvider() + }; + protected localStore!: LocalStore; protected sharedClientState!: SharedClientState; datastore!: Datastore; diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 6e21737380b..738348b016a 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -120,7 +120,6 @@ export class FirestoreClient { ) => Promise = () => Promise.resolve(); _uninitializedComponentsProvider?: { _offline: OfflineComponentProvider; - _offlineKind: 'memory' | 'persistent'; _online: OnlineComponentProvider; }; @@ -139,8 +138,13 @@ export class FirestoreClient { * an async I/O to complete). */ public asyncQueue: AsyncQueue, - private databaseInfo: DatabaseInfo + private databaseInfo: DatabaseInfo, + componentProvider?: { + _offline: OfflineComponentProvider; + _online: OnlineComponentProvider; + } ) { + this._uninitializedComponentsProvider = componentProvider; this.authCredentials.start(asyncQueue, async user => { logDebug(LOG_TAG, 'Received user=', user.uid); await this.authCredentialListener(user); @@ -174,19 +178,6 @@ export class FirestoreClient { this.appCheckCredentialListener = listener; } - /** - * Checks that the client has not been terminated. Ensures that other methods on // - * this class cannot be called after the client is terminated. // - */ - verifyNotTerminated(): void { - if (this.asyncQueue.isShuttingDown) { - throw new FirestoreError( - Code.FAILED_PRECONDITION, - 'The client has already been terminated.' - ); - } - } - terminate(): Promise { this.asyncQueue.enterRestrictedMode(); const deferred = new Deferred(); @@ -253,11 +244,11 @@ export async function setOnlineComponentProvider( ): Promise { client.asyncQueue.verifyOperationInProgress(); - const offlineComponentProvider = await ensureOfflineComponents(client); + const offlineComponents = await ensureOfflineComponents(client); logDebug(LOG_TAG, 'Initializing OnlineComponentProvider'); await onlineComponentProvider.initialize( - offlineComponentProvider, + offlineComponents, client.configuration ); // The CredentialChangeListener of the online component provider takes @@ -665,8 +656,9 @@ function readDocumentViaSnapshotListener( ): Promise { const wrappedObserver = new AsyncObserver({ next: (snap: ViewSnapshot) => { - // Remove query first before passing event to user to avoid + // Mute and remove query first before passing event to user to avoid // user actions affecting the now stale query. + wrappedObserver.mute(); asyncQueue.enqueueAndForget(() => eventManagerUnlisten(eventManager, listener) ); @@ -763,8 +755,9 @@ function executeQueryViaSnapshotListener( ): Promise { const wrappedObserver = new AsyncObserver({ next: snapshot => { - // Remove query first before passing event to user to avoid + // Mute and remove query first before passing event to user to avoid // user actions affecting the now stale query. + wrappedObserver.mute(); asyncQueue.enqueueAndForget(() => eventManagerUnlisten(eventManager, listener) ); diff --git a/packages/firestore/src/lite-api/database.ts b/packages/firestore/src/lite-api/database.ts index 3cc73f6290d..9ea4d4ec52e 100644 --- a/packages/firestore/src/lite-api/database.ts +++ b/packages/firestore/src/lite-api/database.ts @@ -73,8 +73,10 @@ export class Firestore implements FirestoreService { private _settingsFrozen = false; // A task that is assigned when the terminate() is invoked and resolved when - // all components have shut down. - private _terminateTask?: Promise; + // all components have shut down. Otherwise, Firestore is not terminated, + // which can mean either the FirestoreClient is in the process of starting, + // or restarting. + private _terminateTask: Promise | 'notTerminated' = 'notTerminated'; /** @hideconstructor */ constructor( @@ -104,7 +106,7 @@ export class Firestore implements FirestoreService { } get _terminated(): boolean { - return this._terminateTask !== undefined; + return this._terminateTask !== 'notTerminated'; } _setSettings(settings: PrivateSettings): void { @@ -132,12 +134,25 @@ export class Firestore implements FirestoreService { } _delete(): Promise { - if (!this._terminateTask) { + // The `_terminateTask` must be assigned future that completes when + // terminate is complete. The existence of this future puts SDK in state + // that will not accept further API interaction. + if (this._terminateTask === 'notTerminated') { this._terminateTask = this._terminate(); } return this._terminateTask; } + async _restart(): Promise { + // The `_terminateTask` must equal 'notTerminated' after restart to + // signal that client is in a state that accepts API calls. + if (this._terminateTask === 'notTerminated') { + await this._terminate(); + } else { + this._terminateTask = 'notTerminated'; + } + } + /** Returns a JSON-serializable representation of this `Firestore` instance. */ toJSON(): object { return { diff --git a/packages/firestore/src/util/async_observer.ts b/packages/firestore/src/util/async_observer.ts index 5d8b7146851..239d372ea0d 100644 --- a/packages/firestore/src/util/async_observer.ts +++ b/packages/firestore/src/util/async_observer.ts @@ -36,12 +36,18 @@ export class AsyncObserver implements Observer { constructor(private observer: Partial>) {} next(value: T): void { + if (this.muted) { + return; + } if (this.observer.next) { this.scheduleEvent(this.observer.next, value); } } error(error: FirestoreError): void { + if (this.muted) { + return; + } if (this.observer.error) { this.scheduleEvent(this.observer.error, error); } else { @@ -54,12 +60,10 @@ export class AsyncObserver implements Observer { } private scheduleEvent(eventHandler: EventHandler, event: E): void { - if (!this.muted) { - setTimeout(() => { - if (!this.muted) { - eventHandler(event); - } - }, 0); - } + setTimeout(() => { + if (!this.muted) { + eventHandler(event); + } + }, 0); } } diff --git a/packages/firestore/src/util/async_queue_impl.ts b/packages/firestore/src/util/async_queue_impl.ts index ec8bdc62ded..79eb6c23850 100644 --- a/packages/firestore/src/util/async_queue_impl.ts +++ b/packages/firestore/src/util/async_queue_impl.ts @@ -29,7 +29,7 @@ const LOG_TAG = 'AsyncQueue'; export class AsyncQueueImpl implements AsyncQueue { // The last promise in the queue. - private tail: Promise = Promise.resolve(); + private tail: Promise; // A list of retryable operations. Retryable operations are run in order and // retried with backoff. @@ -73,7 +73,8 @@ export class AsyncQueueImpl implements AsyncQueue { this.backoff.skipBackoff(); }; - constructor() { + constructor(tail: Promise = Promise.resolve()) { + this.tail = tail; const document = getDocument(); if (document && typeof document.addEventListener === 'function') { document.addEventListener('visibilitychange', this.visibilityHandler); diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index eda3978dd46..7d025625fe7 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1671,6 +1671,26 @@ apiDescribe('Database', persistence => { }); }); + it('can query after firestore restart', async () => { + return withTestDoc(persistence, async (docRef, firestore) => { + const deferred: Deferred = new Deferred(); + const unsubscribe = onSnapshot(docRef, snapshot => {}, deferred.resolve); + + await firestore._restart(); + + await expect(deferred.promise) + .to.eventually.haveOwnProperty('message') + .equal('Firestore shutting down'); + + // Call should proceed without error. + unsubscribe(); + + await setDoc(docRef, { foo: 'bar' }); + const docSnap = await getDoc(docRef); + expect(docSnap.data()).to.deep.equal({ foo: 'bar' }); + }); + }); + it('query listener throws error on termination', async () => { return withTestDoc(persistence, async (docRef, firestore) => { const deferred: Deferred = new Deferred(); diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index eb0119b5d39..31f9d144142 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -234,10 +234,12 @@ apiDescribe('Validation:', persistence => { validationIt( persistence, 'disallows calling enableIndexedDbPersistence() after use', - db => { - //doc(db, 'foo/bar'); + async db => { + await getDoc(doc(db, 'foo/bar')); expect(() => enableIndexedDbPersistence(db)).to.throw( - 'SDK cache is already specified.' + 'Firestore has already been started and persistence can no ' + + 'longer be enabled. You can only enable persistence before ' + + 'calling any other methods on a Firestore object.' ); } );