search

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

Support for path contexts + misc improvements and fixes #58

Closed mdproctor closed 3 years ago

mdproctor commented 3 years ago

Bug -Carbon returns real paths, this will not map added paths which are symbolic. To address this it turns the real path back into the user provided path. -Fixed some tests, by making them await longer

Improvements -When underlying root paths were deleted, it would not detect and close the Carbon deamon thread or clean up the paths or invalidate the WatchKey. This is now addressed -Can now check if DirectoryWatcher is open or closed. -Added pom.xml so maven sort of works, but it's a quick hack and can be improved.-

Feature -Allow a context value to be associated with a given subtree (i.e. from root path and all child paths), and provided as part of the Event. This avoids having to search for matching roots, to lookup a context value.

mdproctor commented 3 years ago

I should add I only checked that the added maven pom.xml and test work. I notice the SBT scala better stuff fails, it probably needs updating.

mdproctor commented 3 years ago

Btw I'm on gitter, if you want to chat. Left some messages for you there.

mdproctor commented 3 years ago

All suggestions seem reasonable. Rather than reply individually, i'll rework them over next few days and we can re-review them.

Btw one thing I don't think was necessary, was the thread I used to close the Carbon API thread. I only did it, as I wasn't sure if it would deadlock if it was done inplace. If you feel the code won't deadlock, I can do it without needing a thread.

gmethvin commented 3 years ago

I don't think the extra thread is necessary based on my understanding of where the locking is happening. Was there a specific place you were concerned it would deadlock?

mdproctor commented 3 years ago

OK, so I just removed that thread used to close the carbon API, hopefully it's ok. I wasn't sure how this was working, so was being cautious.

gmethvin commented 3 years ago

It looks like there's now a test failing on linux and windows builds:

Any idea what might be causing that?

mdproctor commented 3 years ago

It looks like there's now a test failing on linux and windows builds:

Any idea what might be causing that? I don't have a linux machine, only MacOS, but I'll ask a colleague to look into it.

gmethvin commented 3 years ago

@mdproctor I made some changes that appear to fix the test failures.

The main change is that I moved the tests that delete the root directory into an isMac conditional, so they will only be run for the macOS watcher. There appears to be an inconsistency between platforms in how delete events are reported for the watched roots, so I created issue #59 to track that.

I also removed uses of assumeTrue on the number of events, since those will skip the rest of the test if the condition isn't true. I don't quite understand why you used assumeTrue there, but it seems like those values were based on observed behavior rather than specific guarantees the library is trying to provide, so they aren't really that useful anyway.

Anyway, let me know if those changes look good. If so I'll merge and release.