viper: concurrent map writes panic

Without proper locking, using any of the watch functions is unsafe. This includes WatchConfig and WatchRemoteConfigOnChannel.

Not sure what you use Viper for, but the common use case is to use it for configuration – and to set up that in one thread before the real application starts.

If we should do something on the Viper side it would be to force people into this init -> use by removing all the setters and take all in via constructor funcs.

I agree. I believe basic getters/setters should be thread-safe. There are many uses cases where you’d need to grab configs/values during post-boot runtime of your app (e.g. queues, buffers, etc…)

I use viper in production and this do kill me.

you’d need to grab configs/values during post-boot runtime of your app (e.g. queues, buffers, etc…)

Grab as in “get”, I assume. But what are the use cases for concurrently read/write?

Agree docs should be updated to note it’s not thread-safe. However, it doesn’t necessarily mean viper should be thread-safe as that does come with a performance cost. You can always implement a wrapper around viper of you want concurrent access.

Got this race and that was unexpected… Maybe we need to make a mark that bind it’s not for concurrent use?

I no longer have the code that I was using that would make it happen. But it is very easy to force to happen. The problem really lies in that viper is updating a map in a non concurrent safe way. This triggers it every time for me:

package main

import "github.com/spf13/viper"

func main() {
	go setDefaults()
	setDefaults()
}

func setDefaults() {
	for {
		viper.SetDefault("thing", "value")
	}
}

the issue Im reporting is that viper doesn’t handle concurrency. Regardless of my use case it is an issue, which could be easily solved with a mutex (https://golang.org/pkg/sync/#Mutex).

If you intend to force users into ‘the common use case’ your tool just becomes less useful.