Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add thread safety to all koanf public methods #339

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

musicpulpite
Copy link

@musicpulpite musicpulpite commented Feb 4, 2025

This PR addresses the concerns brought up in Issue #355 and previously in Issue #153.

Since the koanf.confMap is a shared resource that could be accessed concurrently from different goroutines for reading/updating config values, the solution is to acquire and release a sync.RWMutex in each public method that accesses the confMap property.

A more robust and performant solution would be to refactor the existing koanf.confMap map property and all methods in "github.com/knadh/koanf/maps" to use the inherently thread safe sync.Map structure from the standard library. This approach would remove the overhead of adding RWMutex locking to any new public methods and provide more granular read locking in the case of many concurrent read operations where the current approach locks the entire map structure for each read. The obvious downside is that it would require a much more intensive refactor.

@knadh
Copy link
Owner

knadh commented Feb 14, 2025

Looks like the mutex is causing a deadlock: https://github.com/knadh/koanf/actions/runs/13138967660/job/36969992725?pr=339

@musicpulpite
Copy link
Author

musicpulpite commented Feb 18, 2025

Looks like the mutex is causing a deadlock: https://github.com/knadh/koanf/actions/runs/13138967660/job/36969992725?pr=339

Good call. I'll dig into this to see where I messed up and will be sure to write some unit tests to prove that it works as intended (will try to use Go's Data Race Detector) before officially opening the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants