Skip to content

Avoid auth triggers if we haven't yet received the initial user. #2137

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 3 commits into from
Sep 3, 2019

Conversation

rsgowman
Copy link
Member

@rsgowman rsgowman commented Sep 3, 2019

#2078 altered the
initial state of currentUser from undefined to not undefined, which
caused the if statement to unconditionally evaluate to true. This
results in a race condition that could cause some initial writes to the
database to be dropped.

Fixes #2135

#2078 altered the
initial state of currentUser from undefined to not undefined, which
caused the if statement to unconditionally evaluate to true. This
results in a race condition that could cause some initial writes to the
database to be dropped.

Fixes #2135
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Gah! Thanks for tracking this down.

I think this LGTM but we could also probably copy what Android does which would save the extra boolean.

@@ -151,6 +152,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
this.tokenListener = () => {
this.tokenCounter++;
this.currentUser = this.getUser();
this.receivedInitialUser = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just add a this.currentUser = this.getUser(); call below so that it gets initialized in the constructor? This would match Android (https://github.com/firebase/firebase-android-sdk/blob/29a1573ac1c6b7765aa6ed95ceafc95ec3b670f0/firebase-firestore/src/main/java/com/google/firebase/firestore/auth/FirebaseAuthCredentialsProvider.java#L73)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call; done.

@Feiyang1 Feiyang1 merged commit e3d7408 into master Sep 3, 2019
@Feiyang1 Feiyang1 deleted the rsgowman/fix_2135 branch September 3, 2019 23:48
@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firestore writes not working for a Node.js CLI app beyond version 6.0.0. Version 5.11.1 works.
3 participants