Improve watcher #125
Improve watcher #125
Conversation
Profpatsch
force-pushed
the
improve-watcher-2
branch
from
aa4f43f
to
38643b3
Compare
that way it’s not passed as $in to the build rule, which broke the nix invocation.
crossbeam_channel should work as before, there were no functional changes. inotify is a major version, so it changes watching behaviour by a bit, making some of the tests fail.
src/watch.rs
Outdated
| let (watch_tx, watch_rx) = chan::unbounded(); | ||
|
|
||
| let filter = Filter { | ||
| notify: Watcher::new(ntfy_tx, Config::default())?, |
There was a problem hiding this comment.
Is there a reason why the new watcher uses a 30s poll interval, as opposed to the 0.1s from before?
There was a problem hiding this comment.
I think we can probably increase it up to 3 seconds to take some pressure off of bigger projects, but 30s would kind of destroy the idea imo.
There was a problem hiding this comment.
The duration isn't important. I don't have a problem doing something other than the default config, but:
- We aren't using Notify's debounce anymore, so that delay isn't in the config
- I think that everywhere we care about, Notify is using OS level FS events, not polling. Test behavior bears that out.
There was a problem hiding this comment.
yeah, true, I figured out it’s only relevant for the poll backend.
src/watch.rs
Outdated
| logger: logger.clone(), | ||
| }; | ||
|
|
||
| thread::spawn(move || filter.run()); |
There was a problem hiding this comment.
We shouldn’t just spawn threads and ignore their thread handles
There was a problem hiding this comment.
That was my first instinct but we don't have anything to wait on the thread for (all its results are streamed out), so there's little value to joining.
Once the Watcher gets dropped, its side of the channels are dropped, and the Filter will get those errors (e.g. RecvError) and exit its loop.
In the end, I couldn't find a sensible way to make the captured thread handle not dead code, which is why I ignored it.
There was a problem hiding this comment.
Check out the code, I added Async::run_with_stop_signal to kill the event loop on drop.
This function allows us to spawn a cooperative async, which is asked to stop when the async is dropped. We don’t add a timeout when waiting in the drop, meaning if the inner async ignores the channel it might block execution.
Profpatsch
force-pushed
the
improve-watcher-2
branch
from
38643b3
to
faa2736
Compare
Based on code by @nyarly, this debounces watch events before filtering file paths. `notify_debouncer_full` will do the hard part of making sure spurious events are sufficiently removed before we match on them. From the docs: * Only emits a single Rename event if the rename From and To events can be matched * Merges multiple Rename events * Takes Rename events into account and updates paths for events that occurred before the rename event, but which haven't been emitted, yet * Optionally keeps track of the file system IDs all files and stiches rename events together (FSevents, Windows) * Emits only one Remove event when deleting a directory (inotify) * Doesn't emit duplicate create events * Doesn't emit Modify events after a Create event
Profpatsch
force-pushed
the
improve-watcher-2
branch
from
faa2736
to
be5d5d3
Compare
|
The upstream |
Profpatsch
force-pushed
the
improve-watcher-2
branch
from
fd57d92
to
be5d5d3
Compare
`macos_eat_late_notifications` is now less straightforward to implement, because we separated `Watch` from `Filter` and don’t expose the filter channel anymore. So instead let’s pass an optional duration, during which the first event is dropped. This somewhat complicates the event loop, but it can’t be helped, I think??
Profpatsch
force-pushed
the
improve-watcher-2
branch
from
65549c5
to
a020d64
Compare
|
Also lol, searching for that macos panic only brings up https://users.rust-lang.org/t/mysterious-panic-in-macos-cargo-test/77997/1 |
We put every test in a sublogger and add its test name, to aid in debugging.
MacOS has some delay between any change and when the watcher actually notices it. So it would always show the creation of foo when we want to make sure `bar` does not get watched, leading to the test failing. Instead, filter out every file that is not `/bar` here, which should be fine.
|
Actually I think the rename_over_vim test is broken on Linux not mac; we are watching the parent dir, so I’d expect it to notify about the creation of |
|
Ah, nvm, it’s not that, it should be filtered out by the path filtering, so not generate an interesting event, which is the correct case. It’s hard to know with the panic unwinding being broken on current MacOS runners … probably relevant to that is rust-lang/rust#113783 I’d just disable the test on macos for now and hope it fixes itself … maybe |
Expands on #113, fixes the Vim test by debouncing notifications internally (which is easier because we only care about touched paths.)