Skip to content

Commit

Permalink
Implement safe, synchronous destruction in Firestore (#5620)
Browse files Browse the repository at this point in the history
Now that the Executor makes strong guarantees that tasks submitted to it will either be completely run or not start at all once destruction starts (#5547), build a Dispose protocol on top of this to allow Firestore to shut itself down leaf first.

Now that Firestore calls the Dispose chain down to the Executor, it's no longer possible for tasks to run while the instance is partially destructed nor can user callbacks execute after the instance has been destroyed.

This is the other half of the iOS changes required to address firebase/quickstart-unity#638.

There are a few follow-on tasks that this PR does not address:

  * Ownership of FirestoreClient and AsyncQueue are still shared though they no longer need to be.
  * The tests are still overly defensive about keeping different test cases isolated from each other. Much of this can be removed now that we're guaranteed to release resources by the time the test case shuts down (at least in GoogleTest; XCTest remains to be seen).
  • Loading branch information
wilhuff authored May 18, 2020
1 parent e462d04 commit 8ef8b30
Show file tree
Hide file tree
Showing 13 changed files with 414 additions and 200 deletions.
24 changes: 14 additions & 10 deletions Firestore/core/src/api/firestore.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2019 Google
* Copyright 2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -67,13 +67,17 @@ Firestore::Firestore(model::DatabaseId database_id,
}

Firestore::~Firestore() {
std::lock_guard<std::mutex> lock{mutex_};
Dispose();
}

void Firestore::Dispose() {
std::lock_guard<std::mutex> lock(mutex_);

// If the client hasn't been configured yet we don't need to create it just
// to tear it down.
if (!client_) return;

client_->Terminate();
client_->Dispose();
}

const std::shared_ptr<FirestoreClient>& Firestore::client() {
Expand Down Expand Up @@ -156,24 +160,24 @@ void Firestore::WaitForPendingWrites(util::StatusCallback callback) {
}

void Firestore::ClearPersistence(util::StatusCallback callback) {
worker_queue()->EnqueueEvenAfterShutdown([this, callback] {
auto Yield = [=](Status status) {
worker_queue()->EnqueueEvenWhileRestricted([this, callback] {
auto MaybeCallback = [=](Status status) {
if (callback) {
this->user_executor_->Execute([=] { callback(status); });
user_executor_->Execute([=] { callback(status); });
}
};

{
std::lock_guard<std::mutex> lock{mutex_};
if (client_ && !client()->is_terminated()) {
Yield(util::Status(
if (client_ && !client_->is_terminated()) {
MaybeCallback(util::Status(
Error::kErrorFailedPrecondition,
"Persistence cannot be cleared while the client is running."));
return;
}
}

Yield(LevelDbPersistence::ClearPersistence(MakeDatabaseInfo()));
MaybeCallback(LevelDbPersistence::ClearPersistence(MakeDatabaseInfo()));
});
}

Expand All @@ -192,7 +196,7 @@ std::unique_ptr<ListenerRegistration> Firestore::AddSnapshotsInSyncListener(
EnsureClientConfigured();
auto async_listener = AsyncEventListener<Empty>::Create(
client_->user_executor(), std::move(listener));
client_->AddSnapshotsInSyncListener(std::move(async_listener));
client_->AddSnapshotsInSyncListener(async_listener);
return absl::make_unique<SnapshotsInSyncListenerRegistration>(
client_, std::move(async_listener));
}
Expand Down
4 changes: 3 additions & 1 deletion Firestore/core/src/api/firestore.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2019 Google
* Copyright 2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -55,6 +55,8 @@ class Firestore : public std::enable_shared_from_this<Firestore> {

~Firestore();

void Dispose();

const model::DatabaseId& database_id() const {
return database_id_;
}
Expand Down
Loading

0 comments on commit 8ef8b30

Please sign in to comment.