Concurrency-safe way to use viper.WatchConfig()?

[libreoscar]

viper.WatchConfig() store the changed config without any synchronization. Won't it cause data race if there are concurrent config reads in other threads/goroutines?

[ghost]

Yes, there is indeed a data race as I just encountered. Take this for example:

viper.OnConfigChange(func(e fsnotify.Event) {
	if viper.IsSet("globalcd") {
		dur, err := time.ParseDuration(viper.GetString("globalcd"))
		if err != nil {
			log.Printf("Error parsing globalcd value: %s", err)
			return
		}

		if dur > time.Second {
			globalCooldown.Length = dur
		} else {
			globalCooldown.Length = time.Second * 60
		}
	}
}

Race condition on the data read of that value intermittently. So sometimes a value such as: 60s will be read in as 60 which is an error condition for time.ParseDuration as it's lacking the time unit. (exact error: time: missing unit in duration 60)

[7d4b9]

The function registered with OnConfigChange is called in the goroutine of the WatchConfig function event loop and this is a separate goroutine from the one you are using to call viper.

You should not access the viper instance from this registered function as far as viper is not safe for concurrent use.

You should use an external mutex to synchronize you with yourself when you use your viper instance if you want to do that.

package main

import (
	"sync"
	"time"

	"github.com/spf13/viper"
	"github.com/fsnotify/fsnotify"
)

var viperLock sync.Mutex

func main() {

	viperLock.Lock()
	viper.OnConfigChange(func(in fsnotify.Event) {
		viperLock.Lock()
		viper.Set("delay", "10s")
		viperLock.Unlock()
	})
	viperLock.Unlock()

	viperLock.Lock()
	viper.Set("delay", "30s")
	viperLock.Unlock()

	// ...

	viperLock.Lock()
	d := viper.GetDuration("delay")
	viperLock.Unlock()

	time.Sleep(d)
}

[cfergeau]

The function registered with OnConfigChange is called in the goroutine of the WatchConfig function event loop and this is a separate goroutine from the one you are using to call viper.

As I understand it, it's not OnConfigChange which is the problem, but the implementation of viper.WatchConfig:

func (v *Viper) WatchConfig() {
	...
	go func() {
						...
						err := v.ReadInConfig()
						if err != nil {
							log.Printf("error reading config file: %v\n", err)
						}
						if v.onConfigChange != nil {
							v.onConfigChange(event)
						}
...

The v.ReadConfig happens in its own go routine, but can't be synchronized with concurrent uses of v

[Valdenirmezadri]

Hello, i'm make changes to safe concurrency and work for me

https://github.com/Valdenirmezadri/viper

[ArpithaRao]

I am have the same problem. in file viper.go, at line 390:
if v.onConfigChange != nil {
v.onConfigChange(event)
}
I see data race for Read. And at line 341 in the same file, I see data race for write.
Is there a solution for this? This only occurs for the very first time config dynamic change.

[simonkotwicz]

@sagikazarmark thoughts?

[qazwsxedckll]

func (v *Viper) WatchConfig() {
	go func() {
		go func() {
			for {
				select {
				case event, ok := <-watcher.Events:
					if (filepath.Clean(event.Name) == configFile &&
						err := v.ReadInConfig()
						if v.onConfigChange != nil {
							v.onConfigChange(event)
						}
					} else if filepath.Clean(event.Name) == configFile && event.Has(fsnotify.Remove) {
						eventsWG.Done()
						return
					}
				case err, ok := <-watcher.Errors:
				}
			}
		}()
	}()
}


func (v *Viper) ReadInConfig() error {
	config := make(map[string]interface{})

	err = v.unmarshalReader(bytes.NewReader(file), config)
	if err != nil {
		return err
	}

	v.config = config
	return nil
}

This is a race condition to assign v.config and to write configs in onConfigChange without synchronization. No one is looking at this obvious problem?

[zetaab]

there are multiple issues for this race condition in watchConfig. Seems that none cares about the issue