inotify: don't send event for IN_DELETE_SELF when also watching the parent #620
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
…arent This would result in two events being sent. Fixes #299
arp242
added a commit
that referenced
this pull request
Apr 25, 2024
When watching /tmp, w.port.PathIsWatched(name) returned true for /tmp/abc. PathIsWatched() checks: _, found := e.paths[path] return found That e.paths gets set in EventPort.AssociatePath(), which is called by Watcher.associateFile() for every path inside a watched directory. So it can never watch subpaths. Anyway, this seems safe to remove. Could check our own Watcher.dirs state, but this doesn't seem needed (we already have tests for watching the same path more than once). (This came rolling out of #620).
arp242
added a commit
that referenced
this pull request
Apr 26, 2024
When watching /tmp, w.port.PathIsWatched(name) returned true for /tmp/abc. PathIsWatched() checks: _, found := e.paths[path] return found That e.paths gets set in EventPort.AssociatePath(), which is called by Watcher.associateFile() for every path inside a watched directory. So it can never watch subpaths. Anyway, this seems safe to remove. Could check our own Watcher.dirs state, but this doesn't seem needed (we already have tests for watching the same path more than once). (This came rolling out of #620).
giorio94
added a commit
to giorio94/cilium
that referenced
this pull request
Nov 5, 2024
The clustermesh configuration watcher needs to explicitly watch the config files to receive a notification when the underlying file gets updated, if the path points to a symbolic link (e.g., when the folder is mounted from a Kubernetes ConfigMap/Secret). However, the fsnotify library slightly changed its behavior in the latest release [1], and no longer propagates a remove event in case the parent directory is watched as well. The reason being to prevent duplicate events, as it would be generated both by the specific watcher and the parent directory watcher. Still, the parent watcher doesn't emit the remove event in this context, given that the symbolic link is not actually removed. In turn, opening the possibility for race conditions in the current event processing logic, which depends on always correctly re-establishing the watcher. Let's address this by using two separate watchers, one for the config directory itself and one for the individual config files, to ensure that the remove event is correctly emitted when the symbolic link starts pointing to a different file (hence breaking the existing watcher), so that we can re-establish it. [1]: fsnotify/fsnotify#620 Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
added a commit
to giorio94/fsnotify
that referenced
this pull request
Nov 8, 2024
The blamed commit introduced a check to prevent sending a delete event
in case the parent is being watched as well. However, it accesses the
`watches.path` map without synchronization, possibly leading to a data
race in case the same map is accessed in parallel (e.g., by an `Add()`
operation). Let's fix this by grabbing the associated lock before
reading from the map.
Excerpt of the data race trace:
Write at 0x00c0004d8cf0 by goroutine 33:
runtime.mapassign_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:223 +0x0
github.com/fsnotify/fsnotify.(*watches).updatePath()
github.com/fsnotify/fsnotify/backend_inotify.go:163 +0x2b8
github.com/fsnotify/fsnotify.(*inotify).register()
github.com/fsnotify/fsnotify/backend_inotify.go:335 +0x1bd
github.com/fsnotify/fsnotify.(*inotify).add()
github.com/fsnotify/fsnotify/backend_inotify.go:331 +0xee
github.com/fsnotify/fsnotify.(*inotify).AddWith()
github.com/fsnotify/fsnotify/backend_inotify.go:296 +0x405
github.com/fsnotify/fsnotify.(*inotify).Add()
github.com/fsnotify/fsnotify/backend_inotify.go:253 +0x44
github.com/fsnotify/fsnotify.(*Watcher).Add()
github.com/fsnotify/fsnotify/fsnotify.go:313 +0x5ce
Previous read at 0x00c0004d8cf0 by goroutine 32:
runtime.mapaccess2_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:117 +0x0
github.com/fsnotify/fsnotify.(*inotify).readEvents()
github.com/fsnotify/fsnotify/backend_inotify.go:534 +0xd04
github.com/fsnotify/fsnotify.newBufferedBackend.gowrap1()
github.com/fsnotify/fsnotify/backend_inotify.go:195 +0x33
Fixes: e75779f ("inotify: don't send event for IN_DELETE_SELF when also watching the parent (fsnotify#620)")
giorio94
added a commit
to giorio94/fsnotify
that referenced
this pull request
Nov 8, 2024
The blamed commit introduced a check to prevent sending a delete event
in case the parent is being watched as well. However, it accesses the
`watches.path` map without synchronization, possibly leading to a data
race in case the same map is accessed in parallel (e.g., by an `Add()`
operation). Let's fix this by grabbing the associated lock before
reading from the map.
Excerpt of the data race trace:
Write at 0x00c0004d8cf0 by goroutine 33:
runtime.mapassign_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:223 +0x0
github.com/fsnotify/fsnotify.(*watches).updatePath()
github.com/fsnotify/fsnotify/backend_inotify.go:163 +0x2b8
github.com/fsnotify/fsnotify.(*inotify).register()
github.com/fsnotify/fsnotify/backend_inotify.go:335 +0x1bd
github.com/fsnotify/fsnotify.(*inotify).add()
github.com/fsnotify/fsnotify/backend_inotify.go:331 +0xee
github.com/fsnotify/fsnotify.(*inotify).AddWith()
github.com/fsnotify/fsnotify/backend_inotify.go:296 +0x405
github.com/fsnotify/fsnotify.(*inotify).Add()
github.com/fsnotify/fsnotify/backend_inotify.go:253 +0x44
github.com/fsnotify/fsnotify.(*Watcher).Add()
github.com/fsnotify/fsnotify/fsnotify.go:313 +0x5ce
Previous read at 0x00c0004d8cf0 by goroutine 32:
runtime.mapaccess2_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:117 +0x0
github.com/fsnotify/fsnotify.(*inotify).readEvents()
github.com/fsnotify/fsnotify/backend_inotify.go:534 +0xd04
github.com/fsnotify/fsnotify.newBufferedBackend.gowrap1()
github.com/fsnotify/fsnotify/backend_inotify.go:195 +0x33
Fixes: e75779f ("inotify: don't send event for IN_DELETE_SELF when also watching the parent (fsnotify#620)")
aanm
pushed a commit
to cilium/cilium
that referenced
this pull request
Nov 14, 2024
The clustermesh configuration watcher needs to explicitly watch the config files to receive a notification when the underlying file gets updated, if the path points to a symbolic link (e.g., when the folder is mounted from a Kubernetes ConfigMap/Secret). However, the fsnotify library slightly changed its behavior in the latest release [1], and no longer propagates a remove event in case the parent directory is watched as well. The reason being to prevent duplicate events, as it would be generated both by the specific watcher and the parent directory watcher. Still, the parent watcher doesn't emit the remove event in this context, given that the symbolic link is not actually removed. In turn, opening the possibility for race conditions in the current event processing logic, which depends on always correctly re-establishing the watcher. Let's address this by using two separate watchers, one for the config directory itself and one for the individual config files, to ensure that the remove event is correctly emitted when the symbolic link starts pointing to a different file (hence breaking the existing watcher), so that we can re-establish it. [1]: fsnotify/fsnotify#620 Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
added a commit
to giorio94/fsnotify
that referenced
this pull request
Nov 14, 2024
The blamed commit introduced a check to prevent sending a delete event
in case the parent is being watched as well. However, it accesses the
`watches.path` map without synchronization, possibly leading to a data
race in case the same map is accessed in parallel (e.g., by an `Add()`
operation). Let's fix this by grabbing the associated lock before
reading from the map.
Excerpt of the data race trace:
Write at 0x00c0004d8cf0 by goroutine 33:
runtime.mapassign_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:223 +0x0
github.com/fsnotify/fsnotify.(*watches).updatePath()
github.com/fsnotify/fsnotify/backend_inotify.go:163 +0x2b8
github.com/fsnotify/fsnotify.(*inotify).register()
github.com/fsnotify/fsnotify/backend_inotify.go:335 +0x1bd
github.com/fsnotify/fsnotify.(*inotify).add()
github.com/fsnotify/fsnotify/backend_inotify.go:331 +0xee
github.com/fsnotify/fsnotify.(*inotify).AddWith()
github.com/fsnotify/fsnotify/backend_inotify.go:296 +0x405
github.com/fsnotify/fsnotify.(*inotify).Add()
github.com/fsnotify/fsnotify/backend_inotify.go:253 +0x44
github.com/fsnotify/fsnotify.(*Watcher).Add()
github.com/fsnotify/fsnotify/fsnotify.go:313 +0x5ce
Previous read at 0x00c0004d8cf0 by goroutine 32:
runtime.mapaccess2_faststr()
golang.org/toolchain@v0.0.1-go1.23.0.linux-amd64/src/runtime/map_faststr.go:117 +0x0
github.com/fsnotify/fsnotify.(*inotify).readEvents()
github.com/fsnotify/fsnotify/backend_inotify.go:534 +0xd04
github.com/fsnotify/fsnotify.newBufferedBackend.gowrap1()
github.com/fsnotify/fsnotify/backend_inotify.go:195 +0x33
Fixes: e75779f ("inotify: don't send event for IN_DELETE_SELF when also watching the parent (fsnotify#620)")
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 would result in two events being sent.
Fixes #299