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<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);
+}
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<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 @@
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 @@
// 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);
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
