Take RemoteStore offline during user change

Author: schmidt-sebastian

packages/firestore/src/core/firestore_client.ts

@@ -179,9 +179,9 @@ export class FirestoreClient {
           persistenceResult
         ).then(initializationDone.resolve, initializationDone.reject);
       } else {
-        this.asyncQueue.enqueueRetryable(() => {
-          return this.handleCredentialChange(user);
-        });
+        this.asyncQueue.enqueueAndForget(() =>
+          this.remoteStore.handleCredentialChange(user)
+        );
       }
     });
 
@@ -339,13 +339,6 @@ export class FirestoreClient {
     }
   }
 
-  private handleCredentialChange(user: User): Promise<void> {
-    this.asyncQueue.verifyOperationInProgress();
-
-    logDebug(LOG_TAG, 'Credential Changed. Current user: ' + user.uid);
-    return this.syncEngine.handleCredentialChange(user);
-  }
-
   /** Disables the network connection. Pending operations will not complete. */
   disableNetwork(): Promise<void> {
     this.verifyNotTerminated();

packages/firestore/src/core/sync_engine.ts

@@ -860,10 +860,12 @@ export class SyncEngine implements RemoteSyncer {
     );
   }
 
-  async handleCredentialChange(user: User): Promise<void> {
+  async handleUserChange(user: User): Promise<void> {
     const userChanged = !this.currentUser.isEqual(user);
 
     if (userChanged) {
+      logDebug(LOG_TAG, 'User change. New user:', user.toKey());
+
       const result = await this.localStore.handleUserChange(user);
       this.currentUser = user;
 
@@ -879,8 +881,6 @@ export class SyncEngine implements RemoteSyncer {
       );
       await this.emitNewSnapsAndNotifyLocalStore(result.affectedDocuments);
     }
-
-    await this.remoteStore.handleCredentialChange();
   }
 
   enableNetwork(): Promise<void> {

packages/firestore/src/remote/remote_store.ts

@@ -55,6 +55,7 @@ import {
 } from './watch_change';
 import { ByteString } from '../util/byte_string';
 import { isIndexedDbTransactionError } from '../local/simple_db';
+import { User } from '../auth/user';
 
 const LOG_TAG = 'RemoteStore';
 
@@ -756,13 +757,27 @@ export class RemoteStore implements TargetMetadataProvider {
     await this.enableNetwork();
   }
 
-  async handleCredentialChange(): Promise<void> {
+  async handleCredentialChange(user: User): Promise<void> {
+    this.asyncQueue.verifyOperationInProgress();
+
     if (this.canUseNetwork()) {
-      // Tear down and re-create our network streams. This will ensure we get a fresh auth token
-      // for the new user and re-fill the write pipeline with new mutations from the LocalStore
-      // (since mutations are per-user).
+      // Tear down and re-create our network streams. This will ensure we get a
+      // fresh auth token for the new user and re-fill the write pipeline with
+      // new mutations from the LocalStore (since mutations are per-user).
       logDebug(LOG_TAG, 'RemoteStore restarting streams for new credential');
-      await this.restartNetwork();
+
+      this.networkEnabled = false;
+      await this.disableNetworkInternal();
+      this.onlineStateTracker.set(OnlineState.Unknown);
+
+      await this.executeWithRecovery(async () => {
+        await this.syncEngine.handleUserChange(user);
+        await this.enableNetwork();
+      });
+    } else {
+      await this.executeWithRecovery(() =>
+        this.syncEngine.handleUserChange(user)
+      );
     }
   }
 

packages/firestore/src/remote/remote_syncer.ts

@@ -20,6 +20,7 @@ import { DocumentKeySet } from '../model/collections';
 import { MutationBatchResult } from '../model/mutation_batch';
 import { FirestoreError } from '../util/error';
 import { RemoteEvent } from './remote_event';
+import { User } from '../auth/user';
 
 /**
  * An interface that describes the actions the RemoteStore needs to perform on
@@ -65,4 +66,10 @@ export interface RemoteSyncer {
    * the last snapshot.
    */
   getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet;
+
+  /**
+   * Updates all local state to match the pending mutations for the given user.
+   * May be called repeatedly for the same user.
+   */
+  handleUserChange(user: User): Promise<void>;
 }

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

@@ -721,29 +721,37 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         { foo: 'a' },
         { hasLocalMutations: true }
       );
-      return spec()
-        .changeUser('user1')
-        .userSets('collection/key1', { foo: 'a' })
-        .userListens(query)
-        .expectEvents(query, {
-          added: [doc1],
-          fromCache: true,
-          hasPendingWrites: true
-        })
-        .failDatabaseTransactions('Handle user change')
-        .changeUser('user2')
-        .recoverDatabase()
-        .runTimer(TimerId.AsyncQueueRetry)
-        .expectEvents(query, { removed: [doc1], fromCache: true })
-        .failDatabaseTransactions('Handle user change')
-        .changeUser('user1')
-        .recoverDatabase()
-        .runTimer(TimerId.AsyncQueueRetry)
-        .expectEvents(query, {
-          added: [doc1],
-          fromCache: true,
-          hasPendingWrites: true
-        });
+      return (
+        spec()
+          .changeUser('user1')
+          .userSets('collection/key1', { foo: 'a' })
+          .userListens(query)
+          .expectEvents(query, {
+            added: [doc1],
+            fromCache: true,
+            hasPendingWrites: true
+          })
+          .failDatabaseTransactions('Handle user change')
+          .changeUser('user2')
+          // The network is offline due to the failed user change
+          .expectActiveTargets()
+          .recoverDatabase()
+          .runTimer(TimerId.AsyncQueueRetry)
+          .expectActiveTargets({ query })
+          .expectEvents(query, { removed: [doc1], fromCache: true })
+          .failDatabaseTransactions('Handle user change')
+          .changeUser('user1')
+          // The network is offline due to the failed user change
+          .expectActiveTargets()
+          .recoverDatabase()
+          .runTimer(TimerId.AsyncQueueRetry)
+          .expectActiveTargets({ query })
+          .expectEvents(query, {
+            added: [doc1],
+            fromCache: true,
+            hasPendingWrites: true
+          })
+      );
     }
   );
 });

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

@@ -716,17 +716,9 @@ abstract class TestRunner {
 
   private async doChangeUser(user: string | null): Promise<void> {
     this.user = new User(user);
-    const deferred = new Deferred<void>();
-    await this.queue.enqueueRetryable(async () => {
-      try {
-        await this.syncEngine.handleCredentialChange(this.user);
-      } finally {
-        // Resolve the deferred Promise even if the operation failed. This allows
-        // the spec tests to manually retry the failed user change.
-        deferred.resolve();
-      }
-    });
-    return deferred.promise;
+    return this.queue.enqueue(() =>
+      this.remoteStore.handleCredentialChange(this.user)
+    );
   }
 
   private async doFailDatabase(