Stop starting the remote document cache (#1116)

* Stop starting the remote document cache

* Review feedback

* MultiTab -> MultiClient

Author: Greg Soltis

packages/firestore/src/core/firestore_client.ts

@@ -307,7 +307,8 @@ export class FirestoreClient {
         this.clientId,
         this.platform,
         this.asyncQueue,
-        serializer
+        serializer,
+        settings.experimentalTabSynchronization
       );
       this.persistence = persistence;
 
@@ -330,7 +331,7 @@ export class FirestoreClient {
             user
           )
         : new MemorySharedClientState();
-      return persistence.start(settings.experimentalTabSynchronization);
+      return persistence.start();
     });
   }
 

packages/firestore/src/local/indexeddb_persistence.ts

@@ -209,22 +209,27 @@ export class IndexedDbPersistence implements Persistence {
     private readonly clientId: ClientId,
     platform: Platform,
     private readonly queue: AsyncQueue,
-    serializer: JsonProtoSerializer
+    serializer: JsonProtoSerializer,
+    synchronizeTabs: boolean
   ) {
     this.dbName = persistenceKey + IndexedDbPersistence.MAIN_DATABASE;
     this.serializer = new LocalSerializer(serializer);
     this.document = platform.document;
     this.window = platform.window;
+    this.allowTabSynchronization = synchronizeTabs;
+    this.queryCache = new IndexedDbQueryCache(this.serializer);
+    this.remoteDocumentCache = new IndexedDbRemoteDocumentCache(
+      this.serializer,
+      /*keepDocumentChangeLog=*/ this.allowTabSynchronization
+    );
   }
 
   /**
    * Attempt to start IndexedDb persistence.
    *
-   * @param {boolean} synchronizeTabs Whether to enable shared persistence
-   *     across multiple tabs.
    * @return {Promise<void>} Whether persistence was enabled.
    */
-  start(synchronizeTabs?: boolean): Promise<void> {
+  start(): Promise<void> {
     if (!IndexedDbPersistence.isAvailable()) {
       this.persistenceError = new FirestoreError(
         Code.UNIMPLEMENTED,
@@ -234,19 +239,13 @@ export class IndexedDbPersistence implements Persistence {
     }
 
     assert(!this.started, 'IndexedDbPersistence double-started!');
-    this.allowTabSynchronization = !!synchronizeTabs;
-
     assert(this.window !== null, "Expected 'window' to be defined");
 
     return SimpleDb.openOrCreate(this.dbName, SCHEMA_VERSION, createOrUpgradeDb)
       .then(db => {
         this.simpleDb = db;
-        this.queryCache = new IndexedDbQueryCache(this.serializer);
-        this.remoteDocumentCache = new IndexedDbRemoteDocumentCache(
-          this.serializer,
-          /*keepDocumentChangeLog=*/ this.allowTabSynchronization
-        );
       })
+      .then(() => this.startRemoteDocumentCache())
       .then(() => {
         this.attachVisibilityHandler();
         this.attachWindowUnloadHook();
@@ -259,6 +258,12 @@ export class IndexedDbPersistence implements Persistence {
       });
   }
 
+  private startRemoteDocumentCache(): Promise<void> {
+    return this.simpleDb.runTransaction('readonly', ALL_STORES, txn =>
+      this.remoteDocumentCache.start(txn)
+    );
+  }
+
   setPrimaryStateListener(
     primaryStateListener: PrimaryStateListener
   ): Promise<void> {

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -38,7 +38,7 @@ import { PersistencePromise } from './persistence_promise';
 import { RemoteDocumentCache } from './remote_document_cache';
 import { SnapshotVersion } from '../core/snapshot_version';
 import { assert } from '../util/assert';
-import { SimpleDbStore } from './simple_db';
+import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
 
 export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
   /** The last id read by `getNewDocumentChanges()`. */
@@ -60,12 +60,24 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
     return this._lastProcessedDocumentChangeId;
   }
 
-  start(transaction: PersistenceTransaction): PersistencePromise<void> {
+  /**
+   * Starts up the remote document cache.
+   *
+   * Reads the ID of the last  document change from the documentChanges store.
+   * Existing changes will not be returned as part of
+   * `getNewDocumentChanges()`.
+   */
+  // PORTING NOTE: This is only used for multi-tab synchronization.
+  start(transaction: SimpleDbTransaction): PersistencePromise<void> {
     // If there are no existing changes, we set `lastProcessedDocumentChangeId`
     // to 0 since IndexedDb's auto-generated keys start at 1.
     this._lastProcessedDocumentChangeId = 0;
 
-    return documentChangesStore(transaction).iterate(
+    const store = SimpleDb.getStore<
+      DbRemoteDocumentChangesKey,
+      DbRemoteDocumentChanges
+    >(transaction, DbRemoteDocumentChanges.store);
+    return store.iterate(
       { keysOnly: true, reverse: true },
       (key, value, control) => {
         this._lastProcessedDocumentChangeId = key;

packages/firestore/src/local/local_store.ts

@@ -203,11 +203,9 @@ export class LocalStore {
   /** Performs any initial startup actions required by the local store. */
   start(): Promise<void> {
     // TODO(multitab): Ensure that we in fact don't need the primary lease.
-    return this.persistence.runTransaction('Start LocalStore', false, txn => {
-      return this.startMutationQueue(txn).next(() =>
-        this.startRemoteDocumentCache(txn)
-      );
-    });
+    return this.persistence.runTransaction('Start LocalStore', false, txn =>
+      this.startMutationQueue(txn)
+    );
   }
 
   /**
@@ -312,12 +310,6 @@ export class LocalStore {
       });
   }
 
-  private startRemoteDocumentCache(
-    txn: PersistenceTransaction
-  ): PersistencePromise<void> {
-    return this.remoteDocuments.start(txn);
-  }
-
   /* Accept locally generated Mutations and commit them to storage. */
   localWrite(mutations: Mutation[]): Promise<LocalWriteResult> {
     return this.persistence.runTransaction(

packages/firestore/src/local/remote_document_cache.ts

@@ -31,16 +31,6 @@ import { PersistencePromise } from './persistence_promise';
  * known to not exist).
  */
 export interface RemoteDocumentCache {
-  /**
-   * Starts up the remote document cache.
-   *
-   * Reads the ID of the last  document change from the documentChanges store.
-   * Existing changes will not be returned as part of
-   * `getNewDocumentChanges()`.
-   */
-  // PORTING NOTE: This is only used for multi-tab synchronization.
-  start(transaction: PersistenceTransaction): PersistencePromise<void>;
-
   /**
    * Adds or replaces document entries in the cache.
    *

packages/firestore/test/unit/local/indexeddb_persistence.test.ts

@@ -41,6 +41,7 @@ import { PlatformSupport } from '../../../src/platform/platform';
 import { AsyncQueue } from '../../../src/util/async_queue';
 import { SharedFakeWebStorage, TestPlatform } from '../../util/test_platform';
 import { SnapshotVersion } from '../../../src/core/snapshot_version';
+import { PersistenceSettings } from '../../../src/api/database';
 
 const INDEXEDDB_TEST_DATABASE_PREFIX = 'schemaTest/';
 const INDEXEDDB_TEST_DATABASE =
@@ -77,8 +78,9 @@ function withDb(
     });
 }
 
-async function withPersistence(
+async function withCustomPersistence(
   clientId: ClientId,
+  settings: PersistenceSettings,
   fn: (
     persistence: IndexedDbPersistence,
     platform: TestPlatform,
@@ -100,13 +102,46 @@ async function withPersistence(
     clientId,
     platform,
     queue,
-    serializer
+    serializer,
+    settings.experimentalTabSynchronization
   );
 
   await fn(persistence, platform, queue);
   await persistence.shutdown();
 }
 
+async function withPersistence(
+  clientId: ClientId,
+  fn: (
+    persistence: IndexedDbPersistence,
+    platform: TestPlatform,
+    queue: AsyncQueue
+  ) => Promise<void>
+): Promise<void> {
+  return withCustomPersistence(
+    clientId,
+    new PersistenceSettings(/* enabled */ true),
+    fn
+  );
+}
+
+async function withMultiClientPersistence(
+  clientId: ClientId,
+  fn: (
+    persistence: IndexedDbPersistence,
+    platform: TestPlatform,
+    queue: AsyncQueue
+  ) => Promise<void>
+): Promise<void> {
+  return withCustomPersistence(
+    clientId,
+    new PersistenceSettings(/* enabled */ true, {
+      experimentalTabSynchronization: true
+    }),
+    fn
+  );
+}
+
 function getAllObjectStores(db: IDBDatabase): string[] {
   const objectStores: string[] = [];
   for (let i = 0; i < db.objectStoreNames.length; ++i) {
@@ -427,22 +462,20 @@ describe('IndexedDb: allowTabSynchronization', () => {
 
   it('rejects access when synchronization is disabled', () => {
     return withPersistence('clientA', async db1 => {
-      await db1.start(/*synchronizeTabs=*/ false);
+      await db1.start();
       await withPersistence('clientB', async db2 => {
-        await expect(
-          db2.start(/*synchronizeTabs=*/ false)
-        ).to.eventually.be.rejectedWith(
+        await expect(db2.start()).to.eventually.be.rejectedWith(
           'There is another tab open with offline persistence enabled.'
         );
       });
     });
   });
 
   it('grants access when synchronization is enabled', async () => {
-    return withPersistence('clientA', async db1 => {
-      await db1.start(/*synchronizeTabs=*/ true);
-      await withPersistence('clientB', async db2 => {
-        await db2.start(/*synchronizeTabs=*/ true);
+    return withMultiClientPersistence('clientA', async db1 => {
+      await db1.start();
+      await withMultiClientPersistence('clientB', async db2 => {
+        await db2.start();
       });
     });
   });

packages/firestore/test/unit/local/persistence_test_helpers.ts

@@ -72,9 +72,10 @@ export async function testIndexedDbPersistence(
     clientId,
     platform,
     queue,
-    serializer
+    serializer,
+    options.synchronizeTabs
   );
-  await persistence.start(options.synchronizeTabs);
+  await persistence.start();
   return persistence;
 }
 

packages/firestore/test/unit/local/remote_document_cache.test.ts

@@ -32,6 +32,10 @@ import * as persistenceHelpers from './persistence_test_helpers';
 import { TestRemoteDocumentCache } from './test_remote_document_cache';
 import { MaybeDocumentMap } from '../../../src/model/collections';
 import { IndexedDbRemoteDocumentCache } from '../../../src/local/indexeddb_remote_document_cache';
+import {
+  DbRemoteDocumentChangesKey,
+  DbRemoteDocumentChanges
+} from '../../../src/local/indexeddb_schema';
 
 // Helpers for use throughout tests.
 const DOC_PATH = 'a/b';
@@ -78,27 +82,41 @@ describe('IndexedDbRemoteDocumentCache', () => {
           .next(() => cache.removeDocumentChangesThroughChangeId(txn, 1));
       }
     );
+    // We removed the first batch, there should be a single batch remaining.
+    const remainingChangeCount = await persistence.runTransaction(
+      'verify',
+      true,
+      txn => {
+        const store = IndexedDbPersistence.getStore<
+          DbRemoteDocumentChangesKey,
+          DbRemoteDocumentChanges
+        >(txn, DbRemoteDocumentChanges.store);
+        return store.count();
+      }
+    );
+    expect(remainingChangeCount).to.equal(1);
+  });
+
+  it('skips previous changes', async () => {
+    // Add a document to simulate a previous run.
+    let cache = new TestRemoteDocumentCache(
+      persistence,
+      persistence.getRemoteDocumentCache()
+    );
+    await cache.addEntries([doc('a/1', 1, DOC_DATA)]);
     await persistence.shutdown(/* deleteData= */ false);
 
-    // Now make sure that we can only read back the second batch.
+    // Start a new run of the persistence layer
     persistence = await persistenceHelpers.testIndexedDbPersistence({
       synchronizeTabs: true,
       dontPurgeData: true
     });
-
-    // Note that we don't call `start()` on the RemoteDocumentCache. If we did,
-    // the cache would only replay changes that were added after invoking
-    // start().
-    const changedDocs = await persistence.runTransaction(
-      'getNewDocumentChanges',
-      false,
-      txn => {
-        const cache = persistence.getRemoteDocumentCache();
-        return cache.getNewDocumentChanges(txn);
-      }
+    cache = new TestRemoteDocumentCache(
+      persistence,
+      persistence.getRemoteDocumentCache()
     );
-
-    assertMatches([doc('c/1', 3, DOC_DATA)], changedDocs);
+    const changedDocs = await cache.getNextDocumentChanges();
+    assertMatches([], changedDocs);
   });
 
   genericRemoteDocumentCacheTests();
@@ -233,15 +251,6 @@ function genericRemoteDocumentCacheTests(): void {
       changedDocs
     );
   });
-
-  it('start() skips previous changes', async () => {
-    await cache.addEntries([doc('a/1', 1, DOC_DATA)]);
-
-    await cache.start();
-
-    const changedDocs = await cache.getNextDocumentChanges();
-    assertMatches([], changedDocs);
-  });
 }
 
 function assertMatches(

packages/firestore/test/unit/local/test_remote_document_cache.ts

@@ -31,12 +31,6 @@ export class TestRemoteDocumentCache {
     public cache: RemoteDocumentCache
   ) {}
 
-  start(): Promise<void> {
-    return this.persistence.runTransaction('start', false, txn => {
-      return this.cache.start(txn);
-    });
-  }
-
   addEntries(maybeDocuments: MaybeDocument[]): Promise<void> {
     return this.persistence.runTransaction('addEntry', true, txn => {
       return this.cache.addEntries(txn, maybeDocuments);

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -1184,9 +1184,10 @@ class IndexedDbTestRunner extends TestRunner {
       this.clientId,
       this.platform,
       this.queue,
-      serializer
+      serializer,
+      /*synchronizeTabs=*/ true
     );
-    await persistence.start(/*synchronizeTabs=*/ true);
+    await persistence.start();
     return persistence;
   }