testing_hooks.cc: make callbacks synchronous, for simplicity

Author: dconeybe

Firestore/core/src/util/testing_hooks.cc

@@ -16,10 +16,11 @@
 
 #include "Firestore/core/src/util/testing_hooks.h"
 
-#include <future>
+#include <functional>
 #include <mutex>
 #include <type_traits>
 #include <unordered_map>
+#include <utility>
 
 #include "Firestore/core/src/util/no_destructor.h"
 
@@ -88,10 +89,27 @@ TestingHooks::OnExistenceFilterMismatch(
 
 void TestingHooks::NotifyOnExistenceFilterMismatch(
     const ExistenceFilterMismatchInfo& info) {
-  std::lock_guard<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> lock(mutex_);
+
+  // Short-circuit to avoid any unnecessary work if there is nothing to do.
+  if (existence_filter_mismatch_callbacks_.empty()) {
+    return;
+  }
+
+  // Copy the callbacks into a vector so that they can be invoked after
+  // releasing the lock.
+  std::vector<std::shared_ptr<ExistenceFilterMismatchCallback>> callbacks;
   for (auto&& entry : existence_filter_mismatch_callbacks_) {
-    std::async(
-        [info, callback = entry.second]() { callback->operator()(info); });
+    callbacks.push_back(entry.second);
+  }
+
+  // Release the lock so that the callback invocations are done _without_
+  // holding the lock. This avoids deadlock in the case that invocations are
+  // re-entrant.
+  lock.unlock();
+
+  for (std::shared_ptr<ExistenceFilterMismatchCallback> callback : callbacks) {
+    callback->operator()(info);
   }
 }
 

Firestore/core/src/util/testing_hooks.h

@@ -59,11 +59,11 @@ class TestingHooks final {
    * rely on any particular ordering. If a given callback is registered multiple
    * times then it will be notified multiple times, once per registration.
    *
-   * The thread on which the callback occurs is unspecified; listeners should
-   * perform their work as quickly as possible and return to avoid blocking any
-   * critical work. In particular, the listener callbacks should *not* block or
-   * perform long-running operations. Listener callbacks can occur concurrently
-   * with other callbacks on the same and other listeners.
+   * The listener callbacks are performed synchronously in
+   * `NotifyOnExistenceFilterMismatch()`; therefore, listeners should perform
+   * their work as quickly as possible and return to avoid blocking any critical
+   * work. In particular, the listener callbacks should *not* block or perform
+   * long-running operations.
    *
    * The `ExistenceFilterMismatchInfo` reference specified to the callback is
    * only valid during the lifetime of the callback. Once the callback returns
@@ -74,13 +74,16 @@ class TestingHooks final {
    *
    * @return an object whose `Remove()` member function unregisters the given
    * callback; only the first invocation of `Remove()` does anything; all
-   * subsequent invocations do nothing.
+   * subsequent invocations do nothing. Note that due to inherent race
+   * conditions it is technically possible, although unlikely, that callbacks
+   * could still occur _after_ unregistering.
    */
   std::shared_ptr<api::ListenerRegistration> OnExistenceFilterMismatch(
       ExistenceFilterMismatchCallback);
 
   /**
-   * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks.
+   * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks
+   * synchronously.
    * @param info Information about the existence filter mismatch.
    */
   void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&);