-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix potential race during client initialization #4091
Conversation
77605c1
to
46513a1
Compare
auto shared_client = weak_client.lock(); | ||
if (!shared_client) return; | ||
|
||
if (!credentials_initialized) { | ||
credentials_initialized = true; | ||
user_promise->set_value(user); | ||
|
||
shared_client->worker_queue()->Enqueue([shared_client, user, settings] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work.
The whole point of enqueueing outside the credential change listener is that we want to prevent any API call from getting on the async queue before we have the current identity.
The user_promise->get_future().get()
blocks in the body of that callback essentially preventing the async queue from doing anything until we've gotten the callback.
With this change, any API call can now proceed before Auth has called us back and will hit the internals before we're initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, this is actually safe since SetCredentialChangeListener
invokes the callback synchronously on the calling thread when it is first registered. Thus, the client initialization continues to be the first item enqueue on the worker queue.
I added a comment and an assert that makes this more obvious. As part of this, I also had to change credentials_initialized tp be captured by reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
shared_client->Initialize(user, settings); | ||
}); | ||
HARD_ASSERT( | ||
credentials_initialized, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that credentials_changed
is captured by reference it's no longer value to use in the credential_change_listener
once it escapes the current stack frame. Let's move credentials_listener
to be a member of FirestoreClient
to avoid these lifetime issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun. This is fixed.
767cd9d
to
18e3846
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for real this time.
Ports the fix from firebase/firebase-android-sdk@0213e1e, but using the logic of the Web client (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/firestore_client.ts#L184)