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 all 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
1 change: 1 addition & 0 deletions Firestore/core/src/firebase/firestore/core/firestore_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ class FirestoreClient : public std::enable_shared_from_this<FirestoreClient> {
std::chrono::milliseconds initial_gc_delay_ = std::chrono::minutes(1);
std::chrono::milliseconds regular_gc_delay_ = std::chrono::minutes(5);
bool gc_has_run_ = false;
bool credentials_initialized_ = false;
local::LruDelegate* _Nullable lru_delegate_;
util::DelayedOperation lru_callback_;
};
Expand Down
31 changes: 14 additions & 17 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,21 @@
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 = [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);
if (!shared_client->credentials_initialized_) {
shared_client->credentials_initialized_ = true;

// 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 +122,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(
shared_client->credentials_initialized_,
"CredentialChangeListener not invoked during client initialization");

return shared_client;
}
Expand Down