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