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

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.


================
Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+        if (!statusOpt.hasValue())
+          K = DirectoryWatcher::EventKind::Removed;
+      }
----------------
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.


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

https://reviews.llvm.org/D58418