[clang] 2ac0c4b - [DirectoryWatcher] Fix misuse of FSEvents API and data race

[Ben Langmuir via cfe-commits]

Author: Ben Langmuir
Date: 2020-02-11T09:25:38-08:00
New Revision: 2ac0c4b46ee2a3c22b85b4483f2fd4d0fb916720

URL: https://github.com/llvm/llvm-project/commit/2ac0c4b46ee2a3c22b85b4483f2fd4d0fb916720
DIFF: https://github.com/llvm/llvm-project/commit/2ac0c4b46ee2a3c22b85b4483f2fd4d0fb916720.diff

LOG: [DirectoryWatcher] Fix misuse of FSEvents API and data race

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.

Differential Revision: https://reviews.llvm.org/D74371

rdar://59215667

Added: 
    

Modified: 
    clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
    clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp b/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
index 2cae847e7657..7864fb76d160 100644
--- a/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ b/clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -43,24 +43,32 @@ namespace {
 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 {
-    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);
+    // 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;
+      Receiver(
+          DirectoryWatcher::Event(
+              DirectoryWatcher::Event::EventKind::WatcherGotInvalidated, ""),
+          false);
+    });
+
+    // 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;
@@ -217,7 +225,7 @@ llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::creat
   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.
@@ -230,10 +238,6 @@ llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::creat
     // 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);
   };
 

diff  --git a/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp b/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
index 510ade453b0a..650c0fc49764 100644
--- a/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ b/clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -449,3 +449,42 @@ TEST(DirectoryWatcherTest, InvalidatedWatcher) {
 
   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);
+}