Skip to content

Don't switch network status to UNKNOWN during user change #3700

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.

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 7 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/cold-impalas-drop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@firebase/firestore": patch
---

Fixes a bug that caused the client to not raise snapshots from cache if a user change happened while the network connection was disabled.
1 change: 1 addition & 0 deletions packages/firestore/exp/src/api/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export async function setOnlineComponentProvider(
// The CredentialChangeListener of the online component provider takes
// precedence over the offline component provider.
firestore._setCredentialChangeListener(user =>
// TODO(firestoreexp): This should be enqueueRetryable.
firestore._queue.enqueueAndForget(() =>
onlineComponentProvider.remoteStore.handleCredentialChange(user)
)
Expand Down
19 changes: 11 additions & 8 deletions packages/firestore/src/remote/remote_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -791,21 +791,24 @@ export class RemoteStore implements TargetMetadataProvider {

async handleCredentialChange(user: User): Promise<void> {
this.asyncQueue.verifyOperationInProgress();
debugAssert(
!!this.remoteSyncer.handleCredentialChange,
'handleCredentialChange() not set'
);

logDebug(LOG_TAG, 'RemoteStore received new credentials');
const canUseNetwork = 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).
logDebug(LOG_TAG, 'RemoteStore received new credentials');
this.offlineCauses.add(OfflineCause.CredentialChange);

await this.disableNetworkInternal();
this.onlineStateTracker.set(OnlineState.Unknown);
debugAssert(
!!this.remoteSyncer.handleCredentialChange,
'handleCredentialChange() not set'
);
if (canUseNetwork) {
// Don't set the network status to Unknown if we are offline.
this.onlineStateTracker.set(OnlineState.Unknown);
}
await this.remoteSyncer.handleCredentialChange(user);

this.offlineCauses.delete(OfflineCause.CredentialChange);
await this.enableNetworkInternal();
}
Expand Down
17 changes: 17 additions & 0 deletions packages/firestore/test/unit/specs/offline_spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,21 @@ describeSpec('Offline:', [], () => {
);
}
);

specTest('Client stays offline during credential change', [], () => {
// Reproduces a bug that caused the client to switch to OnlineState
// `Unknown` during a credential change.

const query1 = query('collection');
return (
spec()
.disableNetwork()
.changeUser('user1')
.userListens(query1)
// Client is still offline and we raise a `fromCache` event immediately
.expectEvents(query1, {
fromCache: true
})
);
});
});