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

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 18 inline comments as done.
jkorous added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:116
+
+      DirectoryWatcher::EventKind K = DirectoryWatcher::EventKind::Added;
+      if (ievt->mask & IN_MODIFY) {
----------------
jkorous wrote:
> mgorny wrote:
> > I don't think I understand this assumption. Have you any specific event in mind that is supposed to be treated as added here? If not, maybe it'd be better to have no default and instead assert that one of the conditions below matched.
> SGTM. Will rewrite to Optional<> and assert.
Reconsidered and replaced with one-off closure in order not to complicate following code with Optional.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+        if (!statusOpt.hasValue())
+          K = DirectoryWatcher::EventKind::Removed;
+      }
----------------
akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > akyrtzi wrote:
> > > > > mgorny wrote:
> > > > > > akyrtzi wrote:
> > > > > > > mgorny wrote:
> > > > > > > > jkorous wrote:
> > > > > > > > > mgorny wrote:
> > > > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > > > I'll add this comment:
> > > > > > > > > 
> > > > > > > > > // The file might have been removed just after we received the event.
> > > > > > > > Wouldn't that cause removals to be reported twice?
> > > > > > > Not quite sure if it can happen in practice but I'd suggest to accept this as potential occurrence and add it to documentation ("a 'removed' event may be reported twice). I think it is better to handle a definite "fact" (the file doesn't exist) than ignore it and assume the 'removed' event will eventually show up, or try to eliminate the double reporting which would over-complicate the implementation.
> > > > > > > 
> > > > > > > After all, if inotify() is not 100% reliable then there's already the possibility that you'll get a 'removed' event for a file that was not reported as 'added' before.
> > > > > > I see this as a partial workaround for race condition. You 'fix' it if removal happens between inotify reporting the event and you returning it. You don't if removal happens after you push it to events. Therefore, the caller still needs to account for 'added' event being obsolete. I don't really see a purpose in trying to workaround the problem partially here if the caller still needs to account for it. Effectively, you're replacing one normal case and one corner case with one normal case and two corner cases.
> > > > > I was mainly pointing out that the client already has to be prepared for a 'removed' event that does not have an associated 'added' event, regardless of what this code is doing. Therefore a potential double 'removed' event doesn't add complexity to the client.
> > > > > 
> > > > > Could you clarify how the code should handle the inability to get the mod time ? Should it ignore the event ?
> > > > Given the code is supposed to wrap filesystem notification layer, I'd say it should pass the events unmodified and not double-guess what the client expects. The client needs to be prepared for non-atomicity of this anyway.
> > > So it should report an 'added' event but with optional mod-time, essentially reporting that something was added that doesn't exist ?
> > > I'd prefer not to do that because it complicates the client without any real benefit. It was great that you pointed out this part of the code but I'd recommend that if the file is gone we should ignore the 'added' event, instead of complicating the API for a corner case.
> > Except that is technically impossible to avoid reporting something that doesn't exist because it can be removed just after you check for it. So the client needs to *always* support it, otherwise it's fragile to race conditions.
> > 
> > This extra check just covers the short period (-> 0) between reporting and checking. It's needless complexity that doesn't solve the problem. If it does anything, then it gives you false security that you've solved the problem when actually the file may still disappear 1 ns after you've checked that it existed.
> Ok, that's fair, @jkorous I'm fine with removing the mod-time from the DirectoryWatcher API. We can get and report the mod-time at the index-store library layer.
Removed the conditional event kind change and removed ModTime.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154
+    errorMsg += llvm::sys::StrError();
+    return true;
+  };
----------------
dexonsmith wrote:
> jkorous wrote:
> > mgorny wrote:
> > > The return values seem to be confusing. Any reason not to use `true` for success?
> > It seems there's no real consensus * about error/success mapping to bool and since this is just implementation detail (not a public interface) I think it's fine. `llvm::Error` has better semantics but seems a bit heavyweight for this.
> > 
> > * Just tried this: grep -nr "returns true" clang | grep -i error
> Agreed that much of LLVM/Clang uses the C convention where `false`/`0` is success.
> 
> However, I'm curious whether it might be better to thread `llvm::Error` (and/or `llvm::Expected`) through, since you bring it up.  They provide nice assertions when the caller forgets to check the result, they're cheap on success, and there's no true/false ambiguity.
Using `llvm::Error` machinery sounds like a good idea. I'll leave it for a separate patch though.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:94
+                       std::shared_ptr<EventQueue> evtQueue) {
+#define EVT_BUF_LEN (30 * (sizeof(struct inotify_event) + NAME_MAX + 1))
+  char buf[EVT_BUF_LEN] __attribute__((aligned(8)));
----------------
dexonsmith wrote:
> Can this be `constexpr int EventBufferLength` instead of `#define EVT_BUF_LEN`?
Yes, that's a good catch.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:196
+  while (true) {
+    if (close(inotifyFD) == -1 && errno == EINTR)
+      continue;
----------------
mgorny wrote:
> There's some fancy function for this in LLVM. `RetryAfterSignal`, I think.
Didn't know that one. Thanks!


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

https://reviews.llvm.org/D58418