Skip to content
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

Merged
merged 4 commits into from Oct 16, 2019
Merged

Conversation

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Oct 16, 2019

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)

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] {
Copy link
Contributor

@wilhuff wilhuff Oct 16, 2019

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.

Copy link
Member Author

@schmidt-sebastian schmidt-sebastian Oct 16, 2019

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.

@schmidt-sebastian schmidt-sebastian assigned wilhuff and unassigned wilhuff Oct 16, 2019
Copy link
Contributor

@wilhuff wilhuff left a comment

LGTM

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Oct 16, 2019
shared_client->Initialize(user, settings);
});
HARD_ASSERT(
credentials_initialized,
Copy link
Contributor

@wilhuff wilhuff Oct 16, 2019

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.

Copy link
Member Author

@schmidt-sebastian schmidt-sebastian Oct 16, 2019

Choose a reason for hiding this comment

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

Fun. This is fixed.

@schmidt-sebastian schmidt-sebastian removed their assignment Oct 16, 2019
Copy link
Contributor

@wilhuff wilhuff left a comment

LGTM for real this time.

@wilhuff wilhuff merged commit 0f315d6 into master Oct 16, 2019
2 checks passed
@wilhuff wilhuff deleted the mrschmidt/fixrace branch Oct 16, 2019
@firebase firebase locked and limited conversation to collaborators Nov 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants