promise_main.h: fix race conditions

firestore/src/main/promise_main.h

@@ -45,73 +45,55 @@ namespace firestore {
 //
 // `Promise` guarantees that it refers to a valid future backed by `LastResults`
 // array.
-// TODO(b/173819915): fix data races with cleanup.
 template <typename ResultT>
 class Promise {
  public:
- // Creates a future backed by `LastResults` cache.
  Promise(CleanupNotifier* cleanup,
  ReferenceCountedFutureImpl* future_api,
  int identifier)
- : cleanup_{NOT_NULL(cleanup)},
- future_api_{NOT_NULL(future_api)},
- identifier_{identifier},
- handle_{future_api->SafeAlloc<ResultT>(identifier)} {
- RegisterForCleanup();
+ : identifier_(identifier),
+ cleanup_(NOT_NULL(cleanup)),
+ future_api_(NOT_NULL(future_api)),
+ handle_(future_api->SafeAlloc<ResultT>(identifier)) {
+ cleanup_->RegisterObject(this, CleanUp);
  }
 
  ~Promise() {
- std::lock_guard<std::mutex> lock(destruction_mutex_);
- UnregisterForCleanup();
- }
-
- Promise(const Promise& rhs)
- : cleanup_{rhs.cleanup_},
- future_api_{rhs.future_api_},
- identifier_{rhs.identifier_},
- handle_{rhs.handle_} {
- RegisterForCleanup();
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (cleanup_) {
+ cleanup_->UnregisterObject(this);
+ }
  }
 
- Promise(Promise&& rhs) noexcept
- : cleanup_{rhs.cleanup_},
- future_api_{rhs.future_api_},
- identifier_{rhs.identifier_},
- handle_{rhs.handle_} {
- rhs.UnregisterForCleanup();
- rhs.Reset();
+ Promise(const Promise& rhs) : identifier_(rhs.identifier_) {
+ std::lock_guard<std::mutex> lock(rhs.mutex_);
+ if (rhs.cleanup_) {
+ cleanup_ = rhs.cleanup_;
+ future_api_ = rhs.future_api_;
+ handle_ = rhs.handle_;
 
- RegisterForCleanup();
+ cleanup_->RegisterObject(this, CleanUp);
+ }
  }
 
- Promise& operator=(const Promise& rhs) {
- UnregisterForCleanup();
+ Promise(Promise&& rhs) noexcept : identifier_(std::move(rhs.identifier_)) {
+ std::lock_guard<std::mutex> lock(rhs.mutex_);
+ if (rhs.cleanup_) {
+ cleanup_ = std::move(rhs.cleanup_);
+ future_api_ = std::move(rhs.future_api_);
+ handle_ = std::move(rhs.handle_);
 
- cleanup_ = rhs.cleanup_;
- future_api_ = rhs.future_api_;
- identifier_ = rhs.identifier_;
- handle_ = rhs.handle_;
+   rhs.cleanup_->UnregisterObject(&rhs);
+   rhs.cleanup_ = nullptr;
+   rhs.future_api_ = nullptr;
+   rhs.handle_ = {};
 
- RegisterForCleanup();
-
- return *this;
+ cleanup_->RegisterObject(this, CleanUp);
+ }
  }
 
- Promise& operator=(Promise&& rhs) noexcept {
- rhs.UnregisterForCleanup();
- UnregisterForCleanup();
-
- cleanup_ = rhs.cleanup_;
- future_api_ = rhs.future_api_;
- identifier_ = rhs.identifier_;
- handle_ = rhs.handle_;
-
- RegisterForCleanup();
-
- rhs.Reset();
-
- return *this;
- }
+ Promise& operator=(const Promise&) = delete;
+ Promise& operator=(Promise&& rhs) = delete;
 
  // This is only a template function to enable SFINAE. The `Promise` will have
  // either `SetValue(ResultT)` or `SetValue()` defined, based on whether
@@ -124,9 +106,11 @@ class Promise {
  template <typename DummyT = ResultT,
  typename = absl::enable_if_t<!std::is_void<DummyT>::value>>
  void SetValue(DummyT result) {
- if (IsCleanedUp()) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (!future_api_) {
  return;
  }
+
  future_api_->Complete(handle_, NoError(), /*error_message=*/"",
  [&](ResultT* value) {
  // Future API doesn't support moving the value, use
@@ -138,17 +122,21 @@ class Promise {
  template <typename DummyT = ResultT,
  typename = absl::enable_if_t<std::is_void<DummyT>::value>>
  void SetValue() {
- if (IsCleanedUp()) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (!future_api_) {
  return;
  }
+
  future_api_->Complete(handle_, NoError());
  }
 
  void SetError(const util::Status& status) {
  SIMPLE_HARD_ASSERT(
  !status.ok(),
  "To fulfill a promise with 'ok' status, use Promise::SetValue.");
- if (IsCleanedUp()) {
+
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (!future_api_) {
  return;
  }
 
@@ -157,63 +145,32 @@ class Promise {
  }
 
  Future<ResultT> future() {
- if (IsCleanedUp()) {
- return Future<ResultT>{};
+ std::lock_guard<std::mutex> lock(mutex_);
+ if (!future_api_) {
+ return Future<ResultT>();
  }
 
- return Future<ResultT>{future_api_, handle_.get()};
+ return Future<ResultT>(future_api_, handle_.get());
  }
 
  private:
  Promise() = default;
 
  int NoError() const { return static_cast<int>(Error::kErrorOk); }
 
- void Reset() {
- cleanup_ = nullptr;
- future_api_ = nullptr;
- identifier_ = 0;
- handle_ = {};
+ static void CleanUp(void* ptr) {
+ auto* instance = static_cast<Promise*>(ptr);
+ std::lock_guard<std::mutex> lock(instance->mutex_);
+ instance->cleanup_ = nullptr;
+ instance->future_api_ = nullptr;
+ instance->handle_ = {};
  }
 
- // Note: `CleanupFn` is not used because `Promise` is a header-only class, to
- // avoid a circular dependency between headers.
- void RegisterForCleanup() {
- if (IsCleanedUp()) {
- return;
- }
-
- cleanup_->RegisterObject(this, [](void* raw_this) {
- auto* this_ptr = static_cast<Promise*>(raw_this);
- std::unique_lock<std::mutex> lock(this_ptr->destruction_mutex_,
- std::try_to_lock_t());
- // If the destruction mutex is locked, it means the destructor is
- // currently running. In that case, leave the cleanup to destructor;
- // otherwise, trying to acquire the mutex will result in a deadlock
- // (because cleanup is currently holding the cleanup mutex which the
- // destructor will try to acquire to unregister itself from cleanup).
- if (!lock) {
- return;
- }
-
- this_ptr->Reset();
- });
- }
-
- void UnregisterForCleanup() {
- if (IsCleanedUp()) {
- return;
- }
- cleanup_->UnregisterObject(this);
- }
-
- bool IsCleanedUp() const { return cleanup_ == nullptr; }
-
+ mutable std::mutex mutex_;
+ int identifier_ = 0;
  CleanupNotifier* cleanup_ = nullptr;
  ReferenceCountedFutureImpl* future_api_ = nullptr;
- int identifier_ = 0;
  SafeFutureHandle<ResultT> handle_;
- std::mutex destruction_mutex_;
 };
 
 } // namespace firestore

release_build_files/readme.md

@@ -573,6 +573,8 @@ code.
  constructing C++ objects during Objective-C's `+load` method.
 ([#706](https://github.com/firebase/firebase-cpp-sdk/pull/706))
 ([#783](https://github.com/firebase/firebase-cpp-sdk/pull/783))
+ - Firestore: Fix race conditions in `Future` handling.
+ ([#799](https://github.com/firebase/firebase-cpp-sdk/pull/799)).
 
 ### 8.8.0
 - Changes