knadh
Would it be possible to add thread safety directly into this library
Describe the bug Hey @knadh great library. My team is looking to use it more extensively for runtime config fetching at application startup. However, I was wondering if it would be possible to add thread safety directly to this library so that I can safely use it within go routines. I see that you've addressed similar concerns in other issues opened in the past (example). The way I see it, adding RWMutex to access of the internal data structures will make the public methods threadsafe by default while adding minimal overhead to those who continue to use it in a single-thread manner. However, I'd be happy to prove this using some basic benchmarking tests.
To Reproduce Here is a minimal example to illustrate how I would like to use this library in concurrent threads when fetching a large number of separate secrets:
package main
import (
"fmt"
"os"
"runtime"
"sync"
"github.com/defensestation/koanf/providers/secretsmanager"
"github.com/knadh/koanf/v2"
)
const AWS_DEFAULT_REGION = "us-east-2"
func main() {
secretARNs := []string{
"arn1",
"arn2",
"arn3",
"arn4",
}
k := koanf.New("_")
numWorkers := runtime.NumCPU() * 8
var wg sync.WaitGroup
type job struct {
secretARN *string
}
jobsChan := make(chan job, len(secretARNs))
for i := 1; i <= numWorkers; i++ {
wg.Add(1)
go func(jobsChan <-chan job) {
for job := range jobsChan {
secretARN := *job.secretARN
awsRegion, exists := os.LookupEnv("AWS_REGION")
if !exists {
awsRegion = AWS_DEFAULT_REGION
}
provider := secretsmanager.Provider(secretsmanager.Config{
SecretId: secretARN,
AWSRegion: awsRegion,
},
nil,
)
if err := k.Load(provider, nil); err != nil {
panic(fmt.Sprintf("error loading secret concurrently %v", err))
}
}
wg.Done()
}(jobsChan)
}
for _, secretARN := range secretARNs {
jobsChan <- job{secretARN: &secretARN}
}
close(jobsChan)
wg.Wait()
fmt.Println(k.Raw())
return
}
Expected behavior I would like for this code example to be threadsafe automatically but I have been able to trigger a fatal error in tests with almost the exact same configuration:
fatal error: concurrent map writes
fatal error: concurrent map writes
goroutine 28 [running]:
github.com/knadh/koanf/maps.Merge(0x140001c10b0?, 0x1400011c750)
external/gazelle~~go_deps~com_github_knadh_koanf_maps/maps.go:114 +0x194
github.com/knadh/koanf/v2.(*Koanf).merge(0x1400011c7e0, 0x140001c10b0, 0x14000068160)
external/gazelle~~go_deps~com_github_knadh_koanf_v2/koanf.go:416 +0x58
github.com/knadh/koanf/v2.(*Koanf).Load(0x1400011c7e0, {0x1008256b0?, 0x140001c1080?}, {0x0?, 0x0?}, {0x0, 0x0, 0x0?})
Please provide the following information):
- OS: osx
- Koanf Version v2.1.1
Additional context My proposed solution would be to add a RWMutex into the methods like so:
type Koanf struct {
confMap map[string]interface{}
confMapFlat map[string]interface{}
keyMap KeyMap
conf Conf
rwMutex sync.RWMutex
}
and
func (ko *Koanf) merge(c map[string]interface{}, opts *options) error {
ko.rwMutex.Lock()
defer ko.rwMutex.Unlock()
...
and
func (ko *Koanf) Keys() []string {
ko.rwMutex.RLock()
defer ko.rwMutex.RUnlock()
....
Hi @musicpulpite. This rarely comes up for the same reasons highlighted in the linked issue---config is loaded on init and then thrown away.
That said, you're right. An RW lock by default internally can avoid any footguns completely. I can't think of any side effects off the top of my head. Please feel free to send a PR after experimenting.
Hey thanks for getting back to me so quickly. Exactly right, the reason I suggest it at all is because I think it would not impact performance for the majority of users who run this library completely synchronously while enabling those of us who might want to multi-thread to do so safely.
I've already made the change in my forked version to import into my project and you can see a simple benchmark test I slapped together to prove the negligible overhead of adding a RWMutex here.
I'll create a clean fork with just the necessary changes and PR it shortly thanks!
