Fix potential race during client initialization #4091
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/fixrace
branch
from
77605c1 to
46513a1
Compare
| credentials_initialized = true; | ||
| user_promise->set_value(user); | ||
|
|
||
| shared_client->worker_queue()->Enqueue([shared_client, user, settings] { |
There was a problem hiding this comment.
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.
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.
| shared_client->Initialize(user, settings); | ||
| }); | ||
| HARD_ASSERT( | ||
| credentials_initialized, |
There was a problem hiding this comment.
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.
Fun. This is fixed.
schmidt-sebastian
force-pushed
the
mrschmidt/fixrace
branch
from
767cd9d to
18e3846
Compare
3 participants
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)