inotify: don't ignore events for files that don't exist #470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is quite an odd check, leading to an inconsistent event stream, which also doesn't match what other platforms do. If you do a "CREATE + MODIFY + REMOVE" in quick succession then you probably *want* all three events. If you don't want to operate on non-existing files, then you can check this in your application code. You need to do that already, since this check is far from reliable. In the time between this check and the application code doing something with an event the file may have been deleted already. I looked a bit at the history of this, and looks like it was added in 2013 with cc2c34e; issue 36 refers to this issue on the old repo, which mentions it fixes a memory leak: howeyc/fsnotify#36 I can't reproduce that at all; using the CLI from #463 modified to print the memory and running: for i in $(seq 0 10000); { touch $i; rm $i } Memory stays at about 100/110K in both the current main branch and this. So I think it should be safe to remove.
arp242
added a commit
that referenced
this pull request
Jul 25, 2022
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
This was referenced Jul 30, 2022
Closed
arp242
added a commit
that referenced
this pull request
Aug 4, 2022
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
arp242
added a commit
that referenced
this pull request
Aug 6, 2022
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
arp242
added a commit
that referenced
this pull request
Aug 6, 2022
This adds a recursive watcher for inotify; per my suggestion in [1] it
uses the "/..." syntax in the path to indicate recursive watches,
similar to how Go tools work:
w, _ := fsnotify.NewWatcher()
w.Add("some/dir/...")
This will watch "some/dir" as well as any subdirectories of "some/dir".
The upshot of using a syntax like this is that we can:
1. With AddRecursive(path string) adding new Add* methods would become
hard; for example AddMask("path", fsnotify.Create) to only get CREATE
events; we'd then also have to add AddMaskRecursive(). Plus we'd
have to add a RemoveRecursive() as well.
2. With Watcher.SetRecursive() like in #339 it's not possible to add
some paths recursively and others non-recursively, which may be
useful in some cases. Also, it makes it a bit easier to accept user
input; in the CLI or config you can set "dir/..." and just pass that
as-is to fsnotify, without applications having to write special code.
For other watchers it will return ErrRecursionUnsupported for now;
Windows support is already mostly finished in #339, and kqueue can be
added in much the same way as inotify in a future PR.
I also moved all test helpers to helper_test.go, and added a bunch of
"shell-like" functions so you're not forever typing error checks and
filepath.Join(). The new "eventCollector" is also useful in tests to
conveniently collect a slice of events.
TODO:
- Also support recursion in Remove(), and deal with paths added with
"...". I just updated the documentation but didn't actually implement
anything.
- A few test cases don't seem quite right; want to get #470 merged first
as it really confuses things.
- Maybe think of a few more test cases.
[1]: #339 (comment)
|
The |
25 tasks
shogo82148
added a commit
to shogo82148/fsnotify
that referenced
this pull request
Mar 6, 2024
port of fsnotify/fsnotify#470 This is quite an odd check, leading to an inconsistent event stream, which also doesn't match what other platforms do. If you do a "CREATE + MODIFY + REMOVE" in quick succession then you probably *want* all three events. If you don't want to operate on non-existing files, then you can check this in your application code. You need to do that already, since this check is far from reliable. In the time between this check and the application code doing something with an event the file may have been deleted already. I looked a bit at the history of this, and looks like it was added in 2013 with cc2c34e; issue 36 refers to this issue on the old repo, which mentions it fixes a memory leak: howeyc/fsnotify#36 I can't reproduce that at all; using the CLI from #463 modified to print the memory and running: for i in $(seq 0 10000); { touch $i; rm $i } Memory stays at about 100/110K in both the current main branch and this. So I think it should be safe to remove.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is quite an odd check, leading to an inconsistent event stream,
which also doesn't match what other platforms do. If you do a "CREATE +
MODIFY + REMOVE" in quick succession then you probably want all three
events. If you don't want to operate on non-existing files, then you can
check this in your application code.
You need to do that already, since this check is far from reliable. In
the time between this check and the application code doing something
with an event the file may have been deleted already.
I looked a bit at the history of this, and looks like it was added in
2013 with cc2c34e; issue 36 refers to this issue on the old repo, which
mentions it fixes a memory leak: howeyc/fsnotify#36
I can't reproduce that at all; using the CLI from #463 modified to print
the memory and running:
Memory stays at about 100/110K in both the current main branch and this.
So I think it should be safe to remove.