Add thread safety to resolve race conditions in Issues #305 and #335 (#377)

* fix: Add thread safety to core koanf to resolve race conditions

Fixes #305 and #335 by implementing proper synchronization in koanf core.

## Issues Fixed
- Issue #305: File watcher race condition causing empty string reads
- Issue #335: "concurrent map writes" panic during concurrent Load() calls

## Root Cause
The koanf struct's internal maps (confMap, confMapFlat, keyMap) were
accessed concurrently without synchronization, causing race conditions
when multiple goroutines performed read/write operations.

## Solution
- Add sync.RWMutex to Koanf struct for thread safety
- Exclusive locks (Lock) for write operations: merge(), Delete()
- Shared locks (RLock) for read operations: Get(), Keys(), All(), etc.
- Zero breaking changes - identical public API
- Optimized for read-heavy workloads (RWMutex allows concurrent reads)

## Testing
- Added comprehensive race condition tests that reproduce both issues
- TestConcurrentLoadRaceCondition: Reproduces issue #335
- TestFileWatcherRaceCondition: Reproduces issue #305
- TestConcurrentReadWriteMix: Mixed read/write scenarios
- TestConcurrentEdgeCases: Edge case methods (Cut, Copy, etc.)
- All tests pass with -race flag
- All existing functionality tests continue to pass

## Performance Impact
- Zero overhead for single-threaded usage
- Optimal for concurrent reads (multiple readers can proceed simultaneously)
- Minimal contention cost due to RWMutex design

Created using prompts: "both options are not correct, can you look at the underlying code, and see if adding a mutex somewhere is needed" and "we should begin by writing tests that replicate this. and any other race conditions that could occur."

* 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?"

* Simplify file provider synchronization to use only mutex

- Remove atomic operations in favor of simple boolean flags
- Fix potential deadlock in Watch() by releasing lock before goroutine spawn
- Use minimal locking in cleanup goroutine
- Cleaner, more maintainable synchronization pattern
- All tests pass with race detector

* Simplify locking strategy by using localized locking instead of defer patterns

Switch from global defer-based locking to localized locking in read methods to reduce code duplication and improve readability. This eliminates the need for duplicate logic that was added to avoid deadlocks between Sprint() and Keys() methods.

- Keys(), KeyMap(), Get(), Exists() now use minimal lock duration
- Sprint() can safely call Keys() without deadlock concerns
- Remove duplicate key extraction logic from Sprint()
- Maintain defer pattern only where needed for maps.Copy() operations
- All existing tests pass including race condition and deadlock tests

* perf: optimize Sprint() method to reduce lock contention

- Move RLock/RUnlock outside the iteration loop to reduce lock overhead
- Maintain alphabetical sorting behavior for API compatibility
- Eliminate repeated lock acquisitions from O(n) to O(1)
- All tests pass including race condition detection

"optimize Sprint method performance while maintaining thread safety and sorting"

Author: rhnvrm

.github/workflows/test.yml

@@ -26,3 +26,19 @@ jobs:
 
       - name: Run Coverage
         run: go test -v -cover ./...
+
+  race:
+    runs-on: ubuntu-latest
+
+    name: Race Detection Tests
+    steps:
+      - uses: actions/checkout@v4
+ 
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: '1.24'
+          check-latest: true
+
+      - name: Run race detection tests
+        run: go test -race -v github.com/knadh/koanf...

koanf.go

@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"sort"
 	"strconv"
+	"sync"
 
 	"github.com/go-viper/mapstructure/v2"
 	"github.com/knadh/koanf/maps"
@@ -19,6 +20,7 @@ type Koanf struct {
 	confMapFlat map[string]interface{}
 	keyMap      KeyMap
 	conf        Conf
+	mu          sync.RWMutex
 }
 
 // Conf is the Koanf configuration.
@@ -123,46 +125,58 @@ func (ko *Koanf) Load(p Provider, pa Parser, opts ...Option) error {
 // Keys returns the slice of all flattened keys in the loaded configuration
 // sorted alphabetically.
 func (ko *Koanf) Keys() []string {
+	ko.mu.RLock()
 	out := make([]string, 0, len(ko.confMapFlat))
 	for k := range ko.confMapFlat {
 		out = append(out, k)
 	}
+	ko.mu.RUnlock()
 	sort.Strings(out)
 	return out
 }
 
 // KeyMap returns a map of flattened keys and the individual parts of the
 // key as slices. eg: "parent.child.key" => ["parent", "child", "key"].
 func (ko *Koanf) KeyMap() KeyMap {
+	ko.mu.RLock()
 	out := make(KeyMap, len(ko.keyMap))
 	for key, parts := range ko.keyMap {
 		out[key] = make([]string, len(parts))
 		copy(out[key], parts)
 	}
+	ko.mu.RUnlock()
 	return out
 }
 
 // All returns a map of all flattened key paths and their values.
 // Note that it uses maps.Copy to create a copy that uses
 // json.Marshal which changes the numeric types to float64.
 func (ko *Koanf) All() map[string]interface{} {
+	ko.mu.RLock()
+	defer ko.mu.RUnlock()
 	return maps.Copy(ko.confMapFlat)
 }
 
 // Raw returns a copy of the full raw conf map.
 // Note that it uses maps.Copy to create a copy that uses
 // json.Marshal which changes the numeric types to float64.
 func (ko *Koanf) Raw() map[string]interface{} {
+	ko.mu.RLock()
+	defer ko.mu.RUnlock()
 	return maps.Copy(ko.confMap)
 }
 
 // Sprint returns a key -> value string representation
 // of the config map with keys sorted alphabetically.
 func (ko *Koanf) Sprint() string {
 	b := bytes.Buffer{}
-	for _, k := range ko.Keys() {
-		b.WriteString(fmt.Sprintf("%s -> %v\n", k, ko.confMapFlat[k]))
+	keys := ko.Keys()
+	ko.mu.RLock()
+	for _, k := range keys {
+		v := ko.confMapFlat[k]
+		b.WriteString(fmt.Sprintf("%s -> %v\n", k, v))
 	}
+	ko.mu.RUnlock()
 	return b.String()
 }
 
@@ -287,6 +301,9 @@ func (ko *Koanf) UnmarshalWithConf(path string, o interface{}, c UnmarshalConf)
 // Clears all keys/values if no path is specified.
 // Every empty, key on the path, is recursively deleted.
 func (ko *Koanf) Delete(path string) {
+	ko.mu.Lock()
+	defer ko.mu.Unlock()
+
 	// No path. Erase the entire map.
 	if path == "" {
 		ko.confMap = make(map[string]interface{})
@@ -316,11 +333,14 @@ func (ko *Koanf) Get(path string) interface{} {
 	}
 
 	// Does the path exist?
+	ko.mu.RLock()
 	p, ok := ko.keyMap[path]
 	if !ok {
+		ko.mu.RUnlock()
 		return nil
 	}
 	res := maps.Search(ko.confMap, p)
+	ko.mu.RUnlock()
 
 	// Non-reference types are okay to return directly.
 	// Other types are "copied" with maps.Copy or json.Marshal
@@ -370,7 +390,9 @@ func (ko *Koanf) Slices(path string) []*Koanf {
 
 // Exists returns true if the given key path exists in the conf map.
 func (ko *Koanf) Exists(path string) bool {
+	ko.mu.RLock()
 	_, ok := ko.keyMap[path]
+	ko.mu.RUnlock()
 	return ok
 }
 
@@ -404,6 +426,9 @@ func (ko *Koanf) Delim() string {
 }
 
 func (ko *Koanf) merge(c map[string]interface{}, opts *options) error {
+	ko.mu.Lock()
+	defer ko.mu.Unlock()
+
 	maps.IntfaceKeysToStrings(c)
 	if opts.merge != nil {
 		if err := opts.merge(c, ko.confMap); err != nil {

providers/file/file.go

@@ -8,7 +8,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sync/atomic"
+	"sync"
 	"time"
 
 	"github.com/fsnotify/fsnotify"
@@ -19,9 +19,9 @@ type File struct {
 	path string
 	w    *fsnotify.Watcher
 
-	// Using Go 1.18 atomic functions for backwards compatibility.
-	isWatching  uint32
-	isUnwatched uint32
+	// Mutex to protect concurrent access to watcher state
+	mu         sync.Mutex
+	isWatching bool
 }
 
 // Provider returns a file provider.
@@ -42,8 +42,11 @@ func (f *File) Read() (map[string]interface{}, error) {
 // Watch watches the file and triggers a callback when it changes. It is a
 // blocking function that internally spawns a goroutine to watch for changes.
 func (f *File) Watch(cb func(event interface{}, err error)) error {
+	f.mu.Lock()
+	
 	// If a watcher already exists, return an error.
-	if atomic.LoadUint32(&f.isWatching) == 1 {
+	if f.isWatching {
+		f.mu.Unlock()
 		return errors.New("file is already being watched")
 	}
 
@@ -61,10 +64,24 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
 
 	f.w, err = fsnotify.NewWatcher()
 	if err != nil {
+		f.mu.Unlock()
 		return err
 	}
 
-	atomic.StoreUint32(&f.isWatching, 1)
+	f.isWatching = true
+	
+	// Set up the directory watch before releasing the lock
+	err = f.w.Add(fDir)
+	if err != nil {
+		f.w.Close()
+		f.w = nil
+		f.isWatching = false
+		f.mu.Unlock()
+		return err
+	}
+	
+	// Release the lock before spawning goroutine
+	f.mu.Unlock()
 
 	var (
 		lastEvent     string
@@ -77,8 +94,12 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
 			select {
 			case event, ok := <-f.w.Events:
 				if !ok {
-					// Only throw an error if it was not an explicit unwatch.
-					if atomic.LoadUint32(&f.isUnwatched) == 0 {
+					// Only throw an error if we were still supposed to be watching.
+					f.mu.Lock()
+					stillWatching := f.isWatching
+					f.mu.Unlock()
+					
+					if stillWatching {
 						cb(nil, errors.New("fsnotify watch channel closed"))
 					}
 
@@ -126,8 +147,12 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
 			// There's an error.
 			case err, ok := <-f.w.Errors:
 				if !ok {
-					// Only throw an error if it was not an explicit unwatch.
-					if atomic.LoadUint32(&f.isUnwatched) == 0 {
+					// Only throw an error if we were still supposed to be watching.
+					f.mu.Lock()
+					stillWatching := f.isWatching
+					f.mu.Unlock()
+					
+					if stillWatching {
 						cb(nil, errors.New("fsnotify err channel closed"))
 					}
 
@@ -140,17 +165,33 @@ func (f *File) Watch(cb func(event interface{}, err error)) error {
 			}
 		}
 
-		atomic.StoreUint32(&f.isWatching, 0)
-		atomic.StoreUint32(&f.isUnwatched, 0)
-		f.w.Close()
+		f.mu.Lock()
+		f.isWatching = false
+		if f.w != nil {
+			f.w.Close()
+			f.w = nil
+		}
+		f.mu.Unlock()
 	}()
 
-	// Watch the directory for changes.
-	return f.w.Add(fDir)
+	return nil
 }
 
 // Unwatch stops watching the files and closes fsnotify watcher.
 func (f *File) Unwatch() error {
-	atomic.StoreUint32(&f.isUnwatched, 1)
-	return f.w.Close()
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	if !f.isWatching {
+		return nil // Already unwatched
+	}
+	
+	f.isWatching = false
+	if f.w != nil {
+		// Close the watcher to signal the goroutine to stop
+		// The goroutine will handle setting f.w = nil
+		return f.w.Close()
+	}
+	// This state should ideally never be reached - it indicates a bug in the synchronization logic
+	return errors.New("file watcher is in an inconsistent state: isWatching is true but watcher is nil")
 }