[DirectoryWatcher] Fix misuse of FSEvents API and data race

[benlangmuir]

Summary

I observed two bugs in the DirectoryWatcher on macOS

1. We were calling FSEventStreamStop and FSEventStreamInvalidate before we called FSEventStreamStart and FSEventStreamSetDispatchQueue, if the DirectoryWatcher was destroyed before the initial async work was done. This violates the requirements of the FSEvents API.

2. Calls to Receiver could race between the initial work and the invalidation during destruction.

The second issue is easier to see when using TSan.

rdar://59215667

[jkorous]

LGTM

Thanks!

clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp

 /// sleep 5 s
 /// delete foo/bar
 /// receive notification: bar EventKind::Modified
 /// receive notification: bar EventKind::Modified
 /// receive notification: bar EventKind::Removed
 class DirectoryWatcherMac : public clang::DirectoryWatcher {
 public:
   DirectoryWatcherMac(
-      FSEventStreamRef EventStream,
+      dispatch_queue_t Queue, FSEventStreamRef EventStream,
       std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)>
           Receiver,
       llvm::StringRef WatchedDirPath)
-      : EventStream(EventStream), Receiver(Receiver),
+      : Queue(Queue), EventStream(EventStream), Receiver(Receiver),
         WatchedDirPath(WatchedDirPath) {}
 
   ~DirectoryWatcherMac() override {
+    // FSEventStreamStop and Invalidate must be called after Start and
+    // SetDispatchQueue to follow FSEvents API contract. The call to Receiver
+    // also uses Queue to not race with the initial scan.
+    dispatch_sync(Queue, ^{
       stopFSEventStream(EventStream);
       EventStream = nullptr;
-    // Now it's safe to use Receiver as the only other concurrent use would have
-    // been in EventStream processing.
-    Receiver(DirectoryWatcher::Event(
-                 DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""),
-             false);
+      Receiver(
+          DirectoryWatcher::Event(
+    });
+
+    // Balance initial creation.
+    dispatch_release(Queue);
   }
 
 private:
+  dispatch_queue_t Queue;
   FSEventStreamRef EventStream;
   std::function<void(llvm::ArrayRef<Event>, bool)> Receiver;
   const std::string WatchedDirPath;
 };
 
 struct EventStreamContextData {
   std::string WatchedPath;
   std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver;
llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::create(
   if (Path.empty())
     llvm::report_fatal_error(
         "DirectoryWatcher::create can not accept an empty Path.");
 
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
   assert(EventStream && "EventStream expected to be non-null");
 
   std::unique_ptr<DirectoryWatcher> Result =
-      std::make_unique<DirectoryWatcherMac>(EventStream, Receiver, Path);
+      std::make_unique<DirectoryWatcherMac>(Queue, EventStream, Receiver, Path);
 
   // We need to copy the data so the lifetime is ok after a const copy is made
   // for the block.
   const std::string CopiedPath = Path.str();
 
   auto InitWork = ^{
     // We need to start watching the directory before we start scanning in order
     // to not miss any event. By dispatching this on the same serial Queue as
     // the FSEvents will be handled we manage to start watching BEFORE the
     // inital scan and handling events ONLY AFTER the scan finishes.
     FSEventStreamSetDispatchQueue(EventStream, Queue);
     FSEventStreamStart(EventStream);
-    // We need to decrement the ref count for Queue as initialize() will return
-    // and FSEvents has incremented it. Since we have to wait for FSEvents to
-    // take ownership it's the easiest to do it here rather than main thread.
-    dispatch_release(Queue);
     Receiver(getAsFileEvents(scanDirectory(CopiedPath)), /*IsInitial=*/true);
   };
 
   if (WaitForInitialSync) {
     dispatch_sync(Queue, InitWork);
   } else {
     dispatch_async(Queue, InitWork);
   }
 
   return Result;
 }

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
               TestConsumer.consume(Events, IsInitial);
             },
             /*waitForInitialSync=*/true);
     ASSERT_THAT_ERROR(DW.takeError(), Succeeded());
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
 }
+
+TEST(DirectoryWatcherTest, InvalidatedWatcherAsync) {
+  DirectoryWatcherTestFixture fixture;
+  fixture.addFile("a");
+
+  // This test is checking that we get the initial notification for 'a' before
+  // the final 'invalidated'. Strictly speaking, we do not care whether 'a' is
+  // processed or not, only that it is neither racing with, nor after
+  // 'invalidated'. In practice, it is always processed in our implementations.
+  VerifyingConsumer TestConsumer{
+      {{EventKind::Modified, "a"}},
+      {{EventKind::WatcherGotInvalidated, ""}},
+      // We have to ignore these as it's a race between the test process
+      // which is scanning the directory and kernel which is sending
+      // notification.
+      {{EventKind::Modified, "a"}},
+  };
+
+  // A counter that can help detect data races on the event receiver,
+  // particularly if used with TSan. Expected count will be 2 or 3 depending on
+  // whether we get the kernel event or not (see comment above).
+  unsigned Count = 0;
+  {
+    llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+        DirectoryWatcher::create(
+            fixture.TestWatchedDir,
+            [&TestConsumer,
+             &Count](llvm::ArrayRef<DirectoryWatcher::Event> Events,
+                     bool IsInitial) {
+              Count += 1;
+              TestConsumer.consume(Events, IsInitial);
+            },
+            /*waitForInitialSync=*/false);
+    ASSERT_THAT_ERROR(DW.takeError(), Succeeded());
+  } // DW is destructed here.
+
+  checkEventualResultWithTimeout(TestConsumer);
+  ASSERT_TRUE(Count == 2u || Count == 3u);
+}