source/local: Fix fsnotify watcher resource leak #2412
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
size/S
There was a problem hiding this comment.
Pull request overview
This PR addresses a resource leak by ensuring the fsnotify watcher is properly closed when the context is cancelled in the local source implementation. The fix adds a cleanup function that closes the watcher and resets its reference.
Changes:
- Added
cleanupWatcher()method to close the fsnotify watcher and reset the reference to nil - Modified
runNotifier()to defer the cleanup function, ensuring watcher cleanup on context cancellation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Close the fsnotify watcher when the context is cancelled to prevent resource leaks. The watcher reference is also reset to nil to allow proper re-initialization if SetNotifyChannel is called again. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
ArangoGutierrez
force-pushed
the
fix/fswatcher-cleanup
branch
from
554a4f4 to
aaeca24
Compare
k8s-ci-robot
added
size/M
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
source/local/local.go:334
- The
runNotifierfunction accessess.fsWatcher.Eventsands.fsWatcher.Errorswithout holding the mutex lock. SincecleanupWatcher()can sets.fsWatchertonilunder the lock, there's a potential race condition whererunNotifiercould be reading from a nil watcher's channels after cleanup, leading to a panic. Consider holding a local reference to the watcher at the start ofrunNotifierbefore entering the select loop, or add nil checks before accessing the watcher's channels.
case event := <-s.fsWatcher.Events:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/assign @marquiz |
3 participants
Close the fsnotify watcher when the context is cancelled to prevent resource leaks. The watcher reference is also reset to nil to allow proper re-initialization if SetNotifyChannel is called again.