Avoid deadlock during shutdown on Linux/Windows (#5920)

* Avoid deadlock during shutdown on Linux/Windows

Under some circumstances, the Executor can be accessed by a user task
that's running during shutdown.

* Fix running with HAVE_LIBDISPATCH=0 on macOS

Author: wilhuff

Firestore/Source/API/FIRFirestore.mm

@@ -170,8 +170,16 @@ - (void)setSettings:(FIRFirestoreSettings *)settings {
     _settings = settings;
     _firestore->set_settings([settings internalSettings]);
 
+#if HAVE_LIBDISPATCH
     std::unique_ptr<util::Executor> user_executor =
         absl::make_unique<util::ExecutorLibdispatch>(settings.dispatchQueue);
+#else
+    // It's possible to build without libdispatch on macOS for testing purposes.
+    // In this case, avoid breaking the build.
+    std::unique_ptr<util::Executor> user_executor =
+        util::Executor::CreateSerial("com.google.firebase.firestore.user");
+#endif  // HAVE_LIBDISPATCH
+
     _firestore->set_user_executor(std::move(user_executor));
   }
 }

Firestore/core/src/util/executor_std.cc

@@ -74,30 +74,34 @@ ExecutorStd::~ExecutorStd() {
 }
 
 void ExecutorStd::Dispose() {
-  std::lock_guard<std::mutex> lock(mutex_);
+  {
+    std::lock_guard<std::mutex> lock(mutex_);
 
-  // Do nothing if already disposed.
-  if (state_ == nullptr) {
-    return;
-  }
+    // Do nothing if already disposed.
+    if (state_ == nullptr) {
+      return;
+    }
 
-  state_->schedule_.Clear();
-
-  // Enqueue one Task with the kShutdownTag for each worker. Workers will finish
-  // whatever task they're currently working on, execute this task, and then
-  // quit.
-  //
-  // Note that this destructor may be running on a thread managed by this
-  // Executor. This means that these tasks cannot be Awaited, though we do so
-  // indirectly by joining threads if possible. On the thread currently running
-  // this destructor, the kShutdownTag Task will execute after the destructor
-  // completes.
-  for (size_t i = 0; i < worker_thread_pool_.size(); ++i) {
-    PushOnScheduleLocked(Immediate(), kShutdownTag, [] {});
-  }
+    state_->schedule_.Clear();
 
-  state_ = nullptr;
+    // Enqueue one Task with the kShutdownTag for each worker. Workers will
+    // finish whatever task they're currently working on, execute this task,
+    // and then quit.
+    //
+    // Note that this destructor may be running on a thread managed by this
+    // Executor. This means that these tasks cannot be Awaited, though we do so
+    // indirectly by joining threads if possible. On the thread currently
+    // running this destructor, the kShutdownTag Task will execute after the
+    // destructor completes.
+    for (size_t i = 0; i < worker_thread_pool_.size(); ++i) {
+      PushOnScheduleLocked(Immediate(), kShutdownTag, [] {});
+    }
+
+    state_ = nullptr;
+  }
 
+  // Now that `state_` has been released, join any threads while not holding
+  // the lock to avoid deadlocks where the thread tries to access the executor.
   for (std::thread& thread : worker_thread_pool_) {
     // If the current thread is running this destructor, we can't join the
     // thread. Instead detach it and rely on PollingThread to exit cleanly.

Firestore/core/test/unit/util/executor_test.cc

@@ -363,6 +363,44 @@ TEST_P(ExecutorTest, DestructorWaitsForExecutingTasks) {
   ASSERT_EQ(result, "1234");
 }
 
+TEST_P(ExecutorTest, DisposeAvoidsDeadlockingWithCancellation) {
+  Expectation running;
+  Expectation shutdown_started;
+  Expectation cancelled;
+
+  std::string result;
+
+  DelayedOperation operation;
+  operation = executor->Schedule(Executor::Milliseconds(0), 42, [&] {
+    result += "1";
+    running.Fulfill();
+
+    Await(shutdown_started);
+
+    result += "3";
+    operation.Cancel();
+
+    result += "4";
+    cancelled.Fulfill();
+  });
+
+  Expectation shutdown_complete;
+  Async([&] {
+    Await(running);
+    result += "2";
+
+    shutdown_started.Fulfill();
+    executor->Dispose();
+    result += "5";
+
+    shutdown_complete.Fulfill();
+  });
+
+  Await(cancelled);
+  Await(shutdown_complete);
+  ASSERT_EQ(result, "12345");
+}
+
 TEST_P(ExecutorTest, DestructorAvoidsDeadlockWhenDeletingSelf) {
   Expectation complete;
   std::string result;