[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 2 inline comments as done.
jkorous added a comment.

Thanks for taking a look @gribozavr!

I briefly scanned the rest of your comments and I agree with you (or don't really have a strong opinion) in all cases. I'll work on that today.



================
Comment at: clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp:136
+
+            const int PollResult = poll(&PollReq, 1, TimeoutMs);
+            // There are inotify events waiting to be read!
----------------
gribozavr wrote:
> What is the role of the timeout and why does it need to be so small?
The whole idea is that we can't block on `read()` if we ever want to stop watching the directory, release resources (file descriptors, threads) and correctly destruct the DirectoryWatcher instance either
- because of a bug in some other thread in the implementation
- or asynchronous client action (e. g. destructor being called) in main application thread

The timeout adds latency in those scenarios.


================
Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:131
+  // the async event handling picks them up. Can make this test flaky.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100)); // 0.1s
+}
----------------
gribozavr wrote:
> I'm certain this sleep will be flaky on heavily-loaded CI machines.  If you are going to leave it as a sleep, please make it 1s.  But is there really no better way?
That was exactly my thinking! Honestly, I don't know - I wasn't able to come up with any reasonably simple, deterministic approach even on a single platform :(
I eventually picked 0.1s as a tradeoff between slowing the test for everyone and getting less false positives.

The problem as I understand it is that we're making changes and monitoring them asynchronously with no guarantee from the kernel API (true for FSEvents, not 100% about inotify) about when (if) we receive notifications.

If you have any idea for robust testing approach I'd be totally happy to use it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418