Skip to content

Fix potential race during client initialization #4091

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.

Sign up for GitHub

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 4 commits into from
Oct 16, 2019
Merged

Fix potential race during client initialization #4091

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46513a1
Fix potential race during client initialization
schmidt-sebastian Oct 16, 2019
f375633
Add comment and assert
schmidt-sebastian Oct 16, 2019
fe64f03
Add Unreleased back
schmidt-sebastian Oct 16, 2019
18e3846
Don't capture by reference
schmidt-sebastian Oct 16, 2019
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
3 changes: 2 additions & 1 deletion Firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# Unreleased

# v1.6.1
- [changed] Internal improvements.
- [fixed] Fix a race condition that could cause a segmentation fault during
client initialization.

# v1.6.0
- [feature] Added an `addSnapshotsInSyncListener()` method to
Expand Down
26 changes: 13 additions & 13 deletions Firestore/core/src/firebase/firestore/core/firestore_client.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,24 @@
new FirestoreClient(database_info, std::move(credentials_provider),
std::move(user_executor), std::move(worker_queue)));

auto user_promise = std::make_shared<std::promise<User>>();
bool credentials_initialized = false;

std::weak_ptr<FirestoreClient> weak_client(shared_client);
auto credential_change_listener = [credentials_initialized, user_promise,
weak_client](User user) mutable {
auto credential_change_listener = [&credentials_initialized, weak_client,
settings](User user) mutable {
auto shared_client = weak_client.lock();
if (!shared_client) return;

if (!credentials_initialized) {
credentials_initialized = true;
user_promise->set_value(user);

// When we register the credentials listener for the first time,
// it is invoked synchronously on the calling thread. This ensures that
// the first item enqueued on the worker queue is
// `FirestoreClient::Initialize()`.
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
Contributor 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.

shared_client->Initialize(user, settings);
});
} else {
shared_client->worker_queue()->Enqueue([shared_client, user] {
shared_client->worker_queue()->VerifyIsCurrentQueue();
Expand All @@ -119,15 +125,9 @@ new FirestoreClient(database_info, std::move(credentials_provider),
shared_client->credentials_provider_->SetCredentialChangeListener(
credential_change_listener);

// Defer initialization until we get the current user from the
// credential_change_listener. This is guaranteed to be synchronously
// dispatched onto our worker queue, so we will be initialized before any
// subsequently queued work runs.
shared_client->worker_queue()->Enqueue(
[shared_client, user_promise, settings] {
User user = user_promise->get_future().get();
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
Contributor 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.

"CredentialChangeListener not invoked during client initialization");

return shared_client;
}
Expand Down