-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Easy walk-dir like API #175
Comments
Oh, that's a great idea! Certainly something I'll use for vNext, but would also be great if it was implemented for the current branch... not sure how feasible. Thanks for the lovely idea! |
In the mean time, hotwatch looks pretty cool: https://crates.io/crates/hotwatch (good job @francesca64): use hotwatch::{Hotwatch, Event};
let mut hotwatch = Hotwatch::new().expect("Hotwatch failed to initialize.");
hotwatch.watch("war.png", |event: Event| {
if let Event::Write(path) = event {
println!("War has changed.");
}
}).expect("Failed to watch file!"); I liked it so much I put it in the readme. |
Hotwatch API looks nice (especially due to not exposing mpsc channel), but it doesn't handle the most tricky bit: filtering the watched directories. That's a pretty important bit for rust-analyzer: if we just watch the whole directory with Cargo project, we get flooded (up to 100% CPU) with events for the Also cc @vemoo, you might be interested in helping out with this :) |
Another thought: for this API, it would be important to specify the consistency semantics of the events. Obviously, if I receive I think what we want here is quiescent consistency: if there are no changes for a period of time, the last event for each path should correspond to the actual state. In other words, in the sum of the events between two quiescent periods must be equal to the difference between two walkdir traversals. |
Agreed on the filtering.
Probably what that library should do to achieve such consistency is to attempt to
|
Yeah, that's what I've meant. Specifically, this clause about "consistency" was about a sublte bug we had in rust-analyzer. Initially, we did recursive walkdiring on the one thread, and watching on another. We also are interested in the actual "contents" of the files, so both walkdiring thread and watching thread were reading files from disk and sending the results to the single channel, which was the "API". The problem with this setup is that one thread can read file at time t1, another thread can read the file at t2 > t1, but the events in the channel could be in a swapped order. We fixed this by making sure we always read file contents from the first thread, and made the watching thread to only send requests for reading the files to the first one: that way we guarantee that file contents only progresses forward in time. That is, besides infinite ways in which native APIs are broken, there's a fair amount of places where we might accidentally break logical "happens before" relation, and we need to think about that :) |
Yes, I can try to implement this. For v4 at least for inotify shouldn't be too hard since it's already using From taking a quick look at the next v5 code the issue I mention before is more clear thanks to the |
In v4 you get a different implementation per platform that exposes the same interface, in v5 you'd never actually interact with Backends yourself. vNext has very strong separation of abstraction between what Backend implementations do and what the consumers' concerns are, so "vNext Backends" are immutable (makes them super easy to write), and "vNext Notify" itself is a layer that manages adding and removing watches (aka creating and deleting backends) and processing events. In the current design of vNext what you'd do is implement a (The current state is that |
I've started working on the feature for v4, and I have a few questions.
Regarding the v5, I think the backend trait is missing a way to configure the type of watch when creating it, specifically I'm thinking of creating a non recursive folder watch on windows for example. Also if I'm following the code correctly, for recursive watches with backends that don't support it nativelly, it will end up allocating |
vNext is not a viable development target at this time, so don't spend too much
time on it. You bring a good point on the non-recursive-on-windows thing.
More generally, the current trait offers no facility for passing options to
backends, which has other usecases (e.g. event filtering in the kernel). I
do expect that to be resolved by the time it stabilises, but for now the
added complexity was not worth it. Similarly the core likely allocates way
too much at the moment — in that specific case `PathBuf` is easier for
prototyping, but I have thoughts around a descriptor enum to support using
references, inodes, or handles directly, for example. There's many other
such details and questions left... I'd like to get the general shape of the
whole thing going before refining; or at least that's the idea.
I'll have a think about the rest.
I'm not opposed to another method but would it want a `raw_with_filter()`
as well or would the filtering require the debounce?
Also wondering whether adding a variant to that enum would really be a
breaking change... and if it is, whether it would be better to just do
that, instead of a perhaps worse solution.
…On Sat, 9 Feb 2019, 10:31 vemoo ***@***.*** wrote:
I've started working on the feature for v4, and I have a few questions.
-
I think ideally the filtering should in RecursiveMode something like:
pub enum RecursiveMode {
Recursive,
NonRecursive,
Filtered(...)
}
But that would be a breaking change, so I guess the best thing is to
create a new method to create a watcher, new_with_filter maybe?
-
Should we filter files also? It would be a nicer api, but that would
mean possibly turning rename events into create or delete if one of the
names is filtered, that wouldn't be too bad for debounced events but I
don't know how it should be handled for raw events. I was thinking of
initially implementing directory filtering only. Also to make the
configuration more complete I was thinking of exposing a
Fn(&Path)->WalkDir
-
In the current inotify implementation for recursive watching, when a
directory is created it's walked recursively and the found directories are
also watched. But shouldn't we also emit create events for all the found
files and directories?
Regarding the v5, I think the backend trait is missing a way to configure
the type of watch when creating it, specifically I'm thinking of creating a
non recursive folder watch on windows for example. Also if I'm following
the code correctly, for recursive watches with backends that don't support
it nativelly, it will end up allocating Vec<PathBuf> each time a
directory is created.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#175 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJgi3NajM-GDCkvhjj4L0hW4JwFoRKnks5vLey-gaJpZM4aUE6n>
.
|
Can you explain what you mean by
|
|
So that the user could set the available options in struct RecursionFilter<P>
where
P: Fn(&Path) -> bool,
{
filter_dir: P,
follow_links: bool,
} |
Right, sounds good. |
I think I found a way to do it here. I've created an abstraction for the watcher implementation: It's mosly an idea, I haven't tried to implement it yet for any backends. I'll try inotify first, by implementing |
Random thought: in rust-analyzer, we've noticed that on mac we can get Crate event when the write is expected. That is, distinguishing between the kind of event reliably seems tricky. So perhaps instead of
We should have
? |
I have it mostly working for I have yet to add tests for the filtering, but I think the hardest part is done. Adding it to the other watches should be easier since they don't have a recursive implementation that has to be replaced. |
This looks good, I like this.
…On Mon, 18 Feb 2019 at 09:14, vemoo ***@***.***> wrote:
I have it mostly working for inotify (
https://github.com/vemoo/notify/tree/watch-filter). There's 1 test (
watch_recursive_move_out) that fails always and some other that fails
ocasionally. I'll investigate.
I have yet to add tests for the filtering, but I think the hardest part is
done. Adding it to the other watches should be easier since they don't have
a recursive implementation that has to be replaced.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#175 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJgi8Udamae3R-UWcZe1OM0Meheebd5ks5vObgggaJpZM4aUE6n>
.
|
I'm slowly making progress. The test that was failing was a timing issue, I fixed it here: vemoo@51d80bc I ended up implementing filtering for files also, because it wasn't that much more work. This is the filter struct: To implement the filtering I needed to know if a rename event was the "from" or the "to" part, to try to add or remove watches. To keep it simple I check if the path exists and if it does I treat it as a rename "to", otherwise as a rename "from". But inotify and windows distinguish between the two events, so I though that it would be usefull to separate the I also found something in
the debounced events are:
(this is the out put of running the tests https://github.com/vemoo/notify/blob/b1890b91e07f8b7292b8de5bc7cb637a07141868/tests/recursion.rs#L68 and https://github.com/vemoo/notify/blob/b1890b91e07f8b7292b8de5bc7cb637a07141868/tests/recursion.rs#L84 on linux) I've also started working on integrating it in windows, but it's a bit harder than I thought. |
I should have my mac back from the 23rd onwards, so I'll be able to help
thenre.
…On Mon, 25 Feb 2019, 08:09 vemoo, ***@***.***> wrote:
I'm slowly making progress.
The test that was failing was a timing issue, I fixed it here: vemoo@
51d80bc
<vemoo@51d80bc>
I ended up implementing filtering for files also, because it wasn't that
much more work. This is the filter struct: RecursionFilter
<https://github.com/vemoo/notify/blob/watch-filter/src/recursion.rs#L30-L35>
and this is the item to filter on: FilterItem
<https://github.com/vemoo/notify/blob/78c2feb6d9ce440b4ec0da9b1bb7214f3b7734a5/src/recursion.rs#L13-L18>
To implement the filtering I needed to know if a rename event was the
"from" or the "to" part, to try to add or remove watches. To keep it simple
I check if the path exists and if it does I treat it as a rename "to",
otherwise as a rename "from". But inotify and windows distinguish between
the two events, so I though that it would be usefull to separate the
RENAME op in two.
I also found something in Debounce that might be a bug. Given this raw
events (filtered):
[
(
"/tmp/temp_dir.seaDBoBs6epi/dir1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/file1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/file1",
CLOSE_WRITE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir1/file1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir1/file1",
CLOSE_WRITE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir1/file2",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir1/file2",
CLOSE_WRITE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir2",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir2/subdir1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir2/subdir1/file1",
CREATE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir2/subdir1/file1",
CLOSE_WRITE,
None
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/non_ignored_dir",
RENAME,
Some(
2384
)
),
(
"/tmp/temp_dir.seaDBoBs6epi/dir1/subdir2",
RENAME,
Some(
2386
)
)
]
the debounced events are:
[
NoticeRemove(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/non_ignored_dir"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/file1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir1/file1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir1/file2"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir2/subdir1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir2/subdir1/file1"
),
Create(
"/tmp/temp_dir.6u6AFtuQQITu/dir1/subdir2"
)
]
(this is the out put of running the tests
https://github.com/vemoo/notify/blob/b1890b91e07f8b7292b8de5bc7cb637a07141868/tests/recursion.rs#L68
and
https://github.com/vemoo/notify/blob/b1890b91e07f8b7292b8de5bc7cb637a07141868/tests/recursion.rs#L84
on linux)
I think having separate "from" and "to" events will help fix this.
I've also started working on integrating it in windows, but it's a bit
harder than I thought.
For mac I won't be able to try it, unless there's a way to do it from
linux.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#175 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAJgixE11O0NTrdT2RmxyXgEsCbTgj-fks5vQuNogaJpZM4aUE6n>
.
|
It seem this approach doesn't work for windows. Because windows doesn't allow to delete the watched directory neither renaming the parent directory, and in this approach we create a non recursive watch for each directory. There are some tests that I didn't see until after I hit this issue: |
Hi!
We've been using notify in rust-analyzer, and one thing I've noticed that a task like "watch
src/**.rs
glob" requires quite a bit of manual implementation. Specifically, I think two bits are complex:rescan
event which requires repeating the recursive logic againI feel like there's some higher-level API missing here... Ideally I'd love to use something like walkdir:
The text was updated successfully, but these errors were encountered: