[PATCH] D74371: [DirectoryWatcher] Fix misuse of FSEvents API and data race
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2ac0c4b46ee2: [DirectoryWatcher] Fix misuse of FSEvents API
and data race (authored by benlangmuir).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74371/new/
https://reviews.llvm.org/D74371
Files:
clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -449,3 +449,42 @@
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> DW =
+DirectoryWatcher::create(
+fixture.TestWatchedDir,
+[,
+ ](llvm::ArrayRef 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);
+}
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -43,24 +43,32 @@
class DirectoryWatcherMac : public clang::DirectoryWatcher {
public:
DirectoryWatcherMac(
- FSEventStreamRef EventStream,
+ dispatch_queue_t Queue, FSEventStreamRef EventStream,
std::function, 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, bool)> Receiver;
const std::string WatchedDirPath;
@@ -217,7 +225,7 @@
assert(EventStream && "EventStream expected to be non-null");
std::unique_ptr Result =
- std::make_unique(EventStream, Receiver, Path);
+ std::make_unique(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 @@
// 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);
};
[PATCH] D74371: [DirectoryWatcher] Fix misuse of FSEvents API and data race
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74371/new/ https://reviews.llvm.org/D74371 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D74371: [DirectoryWatcher] Fix misuse of FSEvents API and data race
benlangmuir created this revision.
benlangmuir added reviewers: jkorous, akyrtzi.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D74371
Files:
clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
Index: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
===
--- clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
+++ clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
@@ -449,3 +449,42 @@
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> DW =
+DirectoryWatcher::create(
+fixture.TestWatchedDir,
+[,
+ ](llvm::ArrayRef 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);
+}
Index: clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
===
--- clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
+++ clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
@@ -43,24 +43,32 @@
class DirectoryWatcherMac : public clang::DirectoryWatcher {
public:
DirectoryWatcherMac(
- FSEventStreamRef EventStream,
+ dispatch_queue_t Queue, FSEventStreamRef EventStream,
std::function, 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, bool)> Receiver;
const std::string WatchedDirPath;
@@ -217,7 +225,7 @@
assert(EventStream && "EventStream expected to be non-null");
std::unique_ptr Result =
- std::make_unique(EventStream, Receiver, Path);
+ std::make_unique(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 @@
// inital scan and handling events ONLY AFTER the scan finishes.
FSEventStreamSetDispatchQueue(EventStream, Queue);
