Skip to content

Commit 00f18ae

Browse files
committed
fix: Resolve race condition in file provider and TestUnwatchFile
Fixes race conditions detected when running tests with -race flag. ## Root Cause The file provider had a race condition where: 1. Watch() assigns to f.w (fsnotify.Watcher field) 2. Previous watcher's cleanup goroutine calls f.w.Close() 3. These operations could happen concurrently when re-watching after unwatch Additionally, TestUnwatchFile had a race between: - Main test goroutine reading/writing `reloaded` boolean variable - Watch callback goroutine writing to `reloaded` variable ## Solution 1. **File Provider**: Add mutex protection around watcher state changes - Added `mu sync.Mutex` to File struct - Protected Watch() and Unwatch() methods with mutex - Protected cleanup code in watch goroutine with mutex - Added nil checks for defensive programming 2. **TestUnwatchFile**: Use atomic operations instead of plain boolean - Changed `reloaded bool` to `reloaded int32` - Use atomic.StoreInt32/LoadInt32 for thread-safe access - Test now properly verifies re-watching capability after unwatch ## Testing - All watch-related tests pass with race detector: TestWatchFile, TestWatchFileSymlink, TestWatchFileDirectorySymlink, TestUnwatchFile - All submodule tests continue to pass with race detection - CI pattern `github.com/knadh/koanf...` properly tests all submodules Created using prompt: "alright, lets then do a separate commit now to fix the test unwatch file race" and "maybe a better approach will be to add a mutex for file watcher instad?"
1 parent c481aa1 commit 00f18ae

File tree

2 files changed

+28
-10
lines changed

2 files changed

+28
-10
lines changed

providers/file/file.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"fmt"
99
"os"
1010
"path/filepath"
11+
"sync"
1112
"sync/atomic"
1213
"time"
1314

@@ -19,6 +20,9 @@ type File struct {
1920
path string
2021
w *fsnotify.Watcher
2122

23+
// Mutex to protect concurrent access to watcher state
24+
mu sync.Mutex
25+
2226
// Using Go 1.18 atomic functions for backwards compatibility.
2327
isWatching uint32
2428
isUnwatched uint32
@@ -42,6 +46,9 @@ func (f *File) Read() (map[string]interface{}, error) {
4246
// Watch watches the file and triggers a callback when it changes. It is a
4347
// blocking function that internally spawns a goroutine to watch for changes.
4448
func (f *File) Watch(cb func(event interface{}, err error)) error {
49+
f.mu.Lock()
50+
defer f.mu.Unlock()
51+
4552
// If a watcher already exists, return an error.
4653
if atomic.LoadUint32(&f.isWatching) == 1 {
4754
return errors.New("file is already being watched")
@@ -140,9 +147,14 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
140147
}
141148
}
142149

150+
f.mu.Lock()
143151
atomic.StoreUint32(&f.isWatching, 0)
144152
atomic.StoreUint32(&f.isUnwatched, 0)
145-
f.w.Close()
153+
if f.w != nil {
154+
f.w.Close()
155+
f.w = nil
156+
}
157+
f.mu.Unlock()
146158
}()
147159

148160
// Watch the directory for changes.
@@ -151,6 +163,12 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
151163

152164
// Unwatch stops watching the files and closes fsnotify watcher.
153165
func (f *File) Unwatch() error {
166+
f.mu.Lock()
167+
defer f.mu.Unlock()
168+
154169
atomic.StoreUint32(&f.isUnwatched, 1)
155-
return f.w.Close()
170+
if f.w != nil {
171+
return f.w.Close()
172+
}
173+
return nil
156174
}

tests/koanf_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -637,35 +637,35 @@ func TestUnwatchFile(t *testing.T) {
637637
k.Load(f, json.Parser())
638638

639639
// Watch.
640-
reloaded := false
640+
var reloaded int32
641641
f.Watch(func(event interface{}, err error) {
642-
reloaded = true
642+
atomic.StoreInt32(&reloaded, 1)
643643
assert.NoError(err)
644644
})
645645

646646
// Change the file and check whether the watch triggered.
647647
time.Sleep(100 * time.Millisecond)
648648
os.WriteFile(tmpFile, []byte(`{"parent": {"name": "name2"}}`), 0600)
649649
time.Sleep(100 * time.Millisecond)
650-
assert.True(reloaded, "watched file didn't reload")
650+
assert.True(atomic.LoadInt32(&reloaded) == 1, "watched file didn't reload")
651651

652652
// Unwatch the file and verify that the watch didn't trigger.
653653
assert.NoError(f.Unwatch())
654-
reloaded = false
654+
atomic.StoreInt32(&reloaded, 0)
655655
time.Sleep(100 * time.Millisecond)
656656
os.WriteFile(tmpFile, []byte(`{"parent": {"name": "name3"}}`), 0600)
657657
time.Sleep(100 * time.Millisecond)
658-
assert.False(reloaded, "unwatched file reloaded")
658+
assert.False(atomic.LoadInt32(&reloaded) == 1, "unwatched file reloaded")
659659

660660
// Re-watch and check again.
661-
reloaded = false
661+
atomic.StoreInt32(&reloaded, 0)
662662
f.Watch(func(event interface{}, err error) {
663-
reloaded = true
663+
atomic.StoreInt32(&reloaded, 1)
664664
assert.NoError(err)
665665
})
666666
os.WriteFile(tmpFile, []byte(`{"parent": {"name": "name4"}}`), 0600)
667667
time.Sleep(100 * time.Millisecond)
668-
assert.True(reloaded, "watched file didn't reload")
668+
assert.True(atomic.LoadInt32(&reloaded) == 1, "watched file didn't reload")
669669

670670
f.Unwatch()
671671
}

0 commit comments

Comments
 (0)