NotifyWatcher

[utgheith]

An inotify based watcher for Linux NotifyWatcher

Why?

• Doesn't reorder/drop events (assuming no overflow)
• gives us much finer control/information about events

This is an opt-in feature. To enable it:

    os.watch(..., watchConfig = WatchConfig(preferNative=true))

Test cases enable it.

This is a large pull request and it might be wise to split it into multiple parts:

• Randomized testing
• The NotifyWatcher
• ...

To do:
- property based testing
- white box unit tests

Issues:
- You can opt-out of the 'NotifyWatcher' by setting an environment variable OS_WATCH_OLD=1. This is mostly for testing.
- The WatchServiceWatcher fails the randomized rm test after a few iterations when run natively. I've been running the same with the NotifyWatcher for a few days without failure.
- The NotifyWatcher does fail with a small probability when run in Docker.
- The current implementation WatchServiceWatcher has different semantics from the OSX implementation. I made the NotifyService match the WatchServiceWatcher semantics but maybe it's a good idea to make Linux and OSX have identical semantics
- overflow events stop event delivery. I see one of 2 solutions (other than ignoring them): (1) re-process all watches (might lead to unexpected events), (2) report to the caller and let them do what they want (requires an error reporting mechanism)

[lihaoyi]

@utgheith I haven't gone through this in detail, but the overall approach looks reasonable. Honestly the most important thing is the tests: given that the old implementation fails tests and the new implementation passes, that's probably enough to be confident that we're making forward progress

[lefou]

What's the status of this PR? (And who is the main-maintainer of os-lib?) Is anybody OK with me merging it? I'd rather like to, as it would also solve the currently very flaky CI, which blocks PR #53 from getting green, which itself fixes a BSP bug in mill.

[lihaoyi]

@lefou I suppose I'm still the main maintainer, but if you are willing to give it a review and @utgheith is happy with the PR go ahead and merge it

[utgheith]

How about if we started by making it opt-in and enable it by default once it has been tested extensively?

Something like:

os.watch2(...)

or

os.watch.enable_linux_inotify = true

Rationale:

• Gives users (me included) an opportunity to test it in a real-world environment
• Allows us to evolve the API to allow for better error handling (e.g. Error reporting mechanism for os.watch #71)

[utgheith]

Added os.watch.use_linux_inotify to ease the transition. It defaults to false but the os.watch tests set it to true

[lefou]

I haven't reviewed the actual inotify implementation, just the integeration for now.

[lefou]

I don't like the idea of switching this globally. I'd prefer a parameter which enables "native" support. The scaladoc should explain what "native" means for each platform. Also, making this binary compatible would be good idea, e.g. keeping the old def watch signature with sensible default to current behavior.

There is one issue though, as for Mac OS there seems to be currently no choice but only the native implementation. I can't tell if it's because the native version just works better or because the generic version doesn't work at all.

[utgheith]

I did it this way in order to stress the fact that it's an internal implementation detail not intended to survive for a long time and should be neither documented not show up on the public API.

A reasonable alternative is to add an extra argument to os.watch. Naming it something like preferNative would suggest that it's just a preference. It will always be picked on MacOS because it's the only choice.

[utgheith]

3760863

[lihaoyi]

@lefou the generic OSX version uses polling on every watched file, which is as good as "doesn't work at all" on non-trivial folder hierarchies https://macosx-port-dev.openjdk.java.narkive.com/PW3Ir4Q4/file-watching-on-oracle-jdk-on-mac-os-x-and-other-platforms

[lefou]

I like the idea with the WatchConfig, as it avoids future changes to watch API for such kind of tweaks. I don't like the deprecated annotation without any hint though. Also, we should give the user the fine control she wants, and make a flag per platform, when appropriate. And after @lihaoyi s comment I think it will end up with only one flag right now: useLinuxInotify.

[lefou]

I think when we make the new NotifyWatcher optional and disabled by default, this is good enough to merge as-is. I doubt, we will find a reviewer familiar enough with Linux inotify system to review this timely.