Skip to content

Take RemoteStore offline during user change #3193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 22, 2020
Merged

Take RemoteStore offline during user change #3193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00c9136
Take RemoteStore offline during user change
schmidt-sebastian Jun 9, 2020
a1faa32
Add OfflineCause
schmidt-sebastian Jun 18, 2020
87f7137
Rename
schmidt-sebastian Jun 19, 2020
c716d8f
Update spec_test_runner.ts
schmidt-sebastian Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 3 additions & 10 deletions packages/firestore/src/core/firestore_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
);
}
});

Expand Down Expand Up @@ -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();
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,10 +860,12 @@ export class SyncEngine implements RemoteSyncer {
);
}

async handleCredentialChange(user: User): Promise<void> {
async handleUserChange(user: User): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still plenty of other references to handleCredentialChange and CredentialChangeListener. It seems preferable to keep these consistent, no? That way you can search for credentialchange and find all the places we do something about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the RemoteStore and SyncEngine method to handleCredentialChange. LocalStore and SharedClientState still use handleUserChange since SyncEngine "swallows" no-op user changes.

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;

Expand All @@ -879,8 +881,6 @@ export class SyncEngine implements RemoteSyncer {
);
await this.emitNewSnapsAndNotifyLocalStore(result.affectedDocuments);
}

await this.remoteStore.handleCredentialChange();
}

enableNetwork(): Promise<void> {
Expand Down
25 changes: 20 additions & 5 deletions packages/firestore/src/remote/remote_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me uneasy. Previously networkEnabled was only used to signal user intent (around the enableNetwork and disableNetwork calls). Reusing this here means that a public call to enableNetwork can disrupt this logic. I don't think this is desirable, but perhaps that's something you intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the code in restartNetwork() and is needed to silence the asserts that would trigger if the network stream closes without an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is needed to silence assertions in the current implementation, but the larger point I'm after here is that this potentially doesn't actually work.

Consider this sequence of events:

  • There's a credential change
  • IndexedDB fails during syncEngine.handleUserChange, leading that block to be retried later
  • The user calls enableNetwork, disabling the guard rails in this implementation
  • The user performs a listen which may successfully start a stream with the new identity before we've completed the transition within the sync engine.

Or as an alternative to the last step, the device encounters a network transition that prompts restartNetwork to run, again breaking the assumption in here that we won't allow the network to restart until the credential change transition has completed.

What I'm after is that by mixing the intent of the networkEnabled signal between what the user asked for (enableNetwork) and what's possible (IndexedDB is failing, so the streams should be shut down), we open ourselves up to having to reason about these circumstances.

To be clear: the use of networkEnabled in restartNetwork is also suspicious, but it's less immediately problematic because the interval where it's being manipulated is so short. While we're in the background and IndexedDB is failing it could be hours before the sync engine succeeds, which suggests we need a better way to handle this. When this was the only abuse of the user signal this seemed like an ~OK hack. Now that we've got multiple kinds of things potentially interacting with it, the negative effect has multiplied.

So, unless I'm grossly misunderstanding what's happening here, I think I'm proposing we:

  • only track user intent in networkEnabled
  • track network-transition-induced disablement separately from networkEnabled
  • track identity-change-induced disablement separately from both
  • only allow network access when all conditions are independently satisfied.
  • adjust the assertions to account for the fact that there can be multiple reasons why we might disable network access, not just that the user requested it.

This way these events can happen all at once or in any order and we won't accidentally enable the network during an event ordering we didn't anticipate.

Another possible alternative, rather than tracking each condition independently, is to track a cumulative failure counter. I think you still need to track networkEnabled separately because user intent has to be idempotent, but you could make each of the three cases (IndexedDB failed, restart in progress, credential change in progress) increment a count on the initial condition and decrement once it's resolved. Only once all network-preventing actions have completed would we be able to start the stream.

I haven't fully thought this through, but this also seems to help with the case where multiple events of the same type happen before the first event is resolved. For example: if multiple credential change events happen before the syncEngine recovers, we won't start to enable the network after the first completes because the second in-flight credential change prevents it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the long insightful comment! I am convinced that re-using this flag was not the right idea. While we could go with a counter, I have a feeling that it will go out of sync at some point, and we will not be able to figure out why. In the end, I used something similar to what the realtime database does:

firebase-js-sdk/packages/database/src/core/PersistentConnection.ts

Line 813 in 0131e1f

interrupt(reason: string) {
(but instead of strings I used enum constants)

As for having multiple user changes, since they are schedule via the retryable operations, only one user change can be active at a time.

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)
);
}
}

Expand Down
7 changes: 7 additions & 0 deletions packages/firestore/src/remote/remote_syncer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>;
}
54 changes: 31 additions & 23 deletions packages/firestore/test/unit/specs/recovery_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
);
}
);
});
14 changes: 3 additions & 11 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down