Merge pull request #110 from benlangmuir/racy-assert

Fix race in index delegate callback

Author: benlangmuir

Package.swift

@@ -1,4 +1,4 @@
-// swift-tools-version:4.2
+// swift-tools-version:5.0
 
 import PackageDescription
 
@@ -70,7 +70,11 @@ let package = Package(
     .target(
       name: "IndexStoreDB_Database",
       dependencies: ["IndexStoreDB_Core"],
-      path: "lib/Database"),
+      path: "lib/Database",
+      cSettings: [
+        .define("MDB_USE_POSIX_MUTEX", to: "1"),
+        .define("MDB_USE_ROBUST", to: "0"),
+      ]),
 
     // Core index types.
     .target(

lib/Index/IndexSystem.cpp

@@ -50,6 +50,10 @@ class AsyncIndexDelegate : public IndexSystemDelegate {
   AsyncIndexDelegate(std::shared_ptr<IndexSystemDelegate> Other)
     : Others(1, std::move(Other)) {}
 
+  ~AsyncIndexDelegate() {
+    _wait(); // Ensure the queue is drained, since we capture `this`.
+  }
+
   void addDelegate(std::shared_ptr<IndexSystemDelegate> Other) {
     Queue.dispatchSync([&] {
       if (PendingActions)
@@ -60,36 +64,25 @@ class AsyncIndexDelegate : public IndexSystemDelegate {
 
 private:
   virtual void processingAddedPending(unsigned NumActions) override {
-    PendingActions += NumActions;
-
-    if (Others.empty())
-      return;
-    auto LocalOthers = this->Others;
-    Queue.dispatch([LocalOthers, NumActions]{
-      for (auto &other : LocalOthers)
+    Queue.dispatch([this, NumActions]{
+      PendingActions += NumActions;
+      for (auto &other : Others)
         other->processingAddedPending(NumActions);
     });
   }
 
   virtual void processingCompleted(unsigned NumActions) override {
-    assert(NumActions <= PendingActions);
-    PendingActions -= NumActions;
-
-    if (Others.empty())
-      return;
-    auto LocalOthers = this->Others;
-    Queue.dispatch([LocalOthers, NumActions]{
-      for (auto &other : LocalOthers)
+    Queue.dispatch([this, NumActions]{
+      assert(NumActions <= PendingActions);
+      PendingActions -= NumActions;
+      for (auto &other : Others)
         other->processingCompleted(NumActions);
     });
   }
 
   virtual void processedStoreUnit(StoreUnitInfo unitInfo) override {
-    if (Others.empty())
-      return;
-    auto LocalOthers = this->Others;
-    Queue.dispatch([LocalOthers, unitInfo]{
-      for (auto &other : LocalOthers)
+    Queue.dispatch([this, unitInfo]{
+      for (auto &other : Others)
         other->processedStoreUnit(unitInfo);
     });
   }
@@ -98,24 +91,22 @@ class AsyncIndexDelegate : public IndexSystemDelegate {
                                llvm::sys::TimePoint<> outOfDateModTime,
                                OutOfDateTriggerHintRef hint,
                                bool synchronous) override {
-    if (Others.empty())
-      return;
-
     if (synchronous) {
-      for (auto &other : Others)
-        other->unitIsOutOfDate(std::move(unitInfo), outOfDateModTime, hint, true);
+      Queue.dispatchSync([&]{
+        for (auto &other : Others)
+          other->unitIsOutOfDate(std::move(unitInfo), outOfDateModTime, hint, true);
+      });
       return;
     }
 
-    auto LocalOthers = this->Others;
     Queue.dispatch([=]{
-      for (auto &other : LocalOthers)
+      for (auto &other : Others)
         other->unitIsOutOfDate(std::move(unitInfo), outOfDateModTime, hint, false);
     });
   }
 
 public:
-  /// For Testing. Wait for any outstanding async work to finish.
+  /// Public for Testing. Wait for any outstanding async work to finish.
   void _wait() {
     Queue.dispatchSync([]{});
   }