gmethvin / directory-watcher

A cross-platform Java recursive directory watcher, with a JNA macOS watcher and Scala better-files integration
Apache License 2.0
264 stars 34 forks source link

Access denied stops walking file tree #81

Closed zenios closed 2 years ago

zenios commented 2 years ago

Hi

When walking a folder using Files.walkFileTree if a file / folder cannot be accessed an exception is thrown and the whole walking is finished.

It might be good to handle such cases by overriding visitFileFailed and continue instead of breaking

gmethvin commented 2 years ago

That’s definitely not ideal. What do you think the right behavior is? I wonder if we should just always ignore the error, of if we should notify the user somehow.

zenios commented 2 years ago

The default should be Continue and at the same time give the option to the user to pass a handler on construction something like the following

DirectoryWatcher.builder().onVisitFileFailed(e -> FileVisitResult.TERMINATE)

On Sat, Jul 23, 2022 at 3:28 PM Greg Methvin @.***> wrote:

That’s definitely not ideal. What do you think the right behavior is? I wonder if we should just always ignore the error, of if we should notify the user somehow.

— Reply to this email directly, view it on GitHub https://github.com/gmethvin/directory-watcher/issues/81#issuecomment-1193117674, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAV3YJG66D7HXFROSULUODVVPQP5ANCNFSM54MRCXCQ . You are receiving this because you authored the thread.Message ID: @.***>

gmethvin commented 2 years ago

That sounds reasonable. Do you want to send a PR?

zenios commented 2 years ago

Sure will try to work over the weekend.not sure how a test case can be written though for such cases

On Sat, 23 Jul 2022, 16:09 Greg Methvin, @.***> wrote:

That sounds reasonable. Do you want to send a PR?

— Reply to this email directly, view it on GitHub https://github.com/gmethvin/directory-watcher/issues/81#issuecomment-1193123246, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAV3YM5BJUUSA5IEM7O2STVVPVJDANCNFSM54MRCXCQ . You are receiving this because you modified the open/close state.Message ID: @.***>

zenios commented 2 years ago

Hey

I had a look over the code. So the issue happens at recursiveVisitFiles. That method is being called plenty of times

  1. Directory created while watching
  2. Directory watcher init to calculate hashes
  3. Directory watcher init to register folders
  4. Some more times inside MacOSXListeningWatchService which didn't check to see exactly what it does

Since the method might be called multiple times from different functions for same paths we need a consistent result. We cant let the consumer decide ( We want to avoid situations in which when calculating hashes the consumer returns SKIP_SIBLINGS and on folder registration the consumer returns TERMINATE )

Based on the above our 3 remaining options are

Based on this small analysis i think that we should enforce CONTINUE and done with it.

As for notifying the user, since the user has no control over what happens there is no point (Nothing the user can do except logging and for that case we can use the already injected logger)

Looking forward to your comments

gmethvin commented 2 years ago

I agree CONTINUE is probably the most useful behavior in most cases. It would be useful to at least log a warning there though.

I'm not convinced that a notification mechanism isn't useful though. What if the program using the watcher wants to ask the end user to fix their permissions?

gmethvin commented 2 years ago

I do agree though that it's kind of a pain to go through and add a callback to every single case where we use the file visitor. Why don't we start with just continue and log, then we can look at incremental improvements from there?