concurrent map writes panic

[lyondhill]

Since the update in go to the map concurrent write check viper occasionally fails

fatal error: concurrent map writes                                                                                                                                                                                                                                              

goroutine 7 [running]:
runtime.throw(0x5c3575, 0x15)
    /data/go/src/runtime/panic.go:566 +0x95 fp=0xc420042c70 sp=0xc420042c50
runtime.mapassign1(0x527840, 0xc4202fc5d0, 0xc420042d90, 0xc420042d80)
    /data/go/src/runtime/hashmap.go:458 +0x8ef fp=0xc420042d58 sp=0xc420042c70
github.com/nanobox-io/nanobox/vendor/github.com/spf13/viper.(*Viper).SetDefault(0xc420312000, 0x5c3fef, 0x16, 0x4ffcc0, 0xc4202fa590)
    /app/.gopath/src/github.com/nanobox-io/nanobox/vendor/github.com/spf13/viper/viper.go:881 +0xb8 fp=0xc420042db0 sp=0xc420042d58

The line numbers no longer match the current master branch but the issue for this specific one would be found here now: https://github.com/spf13/viper/blob/master/viper.go#L1052

[bep]

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.

[lyondhill]

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.

[bep]

I do not intend to do anything about this, I was just suggesting.This is a @spf13 project, he will probably decide if it is worth the extra work/tests to sprinkle the project with mutex locking to support a rare use case.

[ghost]

bump - @lyondhill , do you have a test case you can share? Thanks.

[lyondhill]

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")
    }
}

[alexanderbez]

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...)

[bep]

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?

[ghost]

Viper is not thread-safe. Using some typical Go techniques, I re-implemented a small core of Viper with JSON support and support for basic Consul watchers (not the most ideal implementation, since it uses polling.) You can find the small sample rewrite here: github.com/rbastic/mvga

FYI, my use case is for microservices requiring dynamic configuration reloading.

[claudiofahey]

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

[painhardcore]

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

[stanislav-bios-baranov]

Got same problem, but race appears in a place, which is totally unexpected to face it: UnmarshalKey. My expectation was, that unmarshaling part of config into external struct, should not change any internal structures inside viper package.

Here is small test, which fails for me in 50% of cases:

package main

import (
    "fmt"
    "testing"

    "github.com/stretchr/testify/require"

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

type Setting struct {
    TestValue *string `json:"testValue,omitempty" mapstructure:"testvalue"`
}

func TestConcurrency(t *testing.T) {

    content := []byte(`{"settings": {"A": {"value": "value A"}, "B": {"value": "value B"}}}`)

    aferoFS := afero.NewMemMapFs()
    require.NoError(t, afero.WriteFile(aferoFS, "conf.json", content, 0755))

    v := viper.New()

    v.SetConfigType("json")
    v.SetFs(aferoFS)
    v.SetConfigFile("conf.json")
    require.NoError(t, v.ReadInConfig())

    for i := 1; i < 100; i++ {
        t.Run(fmt.Sprintf("flow #%d", i), func(tt *testing.T) {
            tt.Parallel()
            settings := make(map[string]*Setting)
            require.NotPanics(t, func() {
                require.NoError(t, v.UnmarshalKey("settings", &settings))
            })
        })
    }

}

In my case problem is with insensitiviseMaps call from UnmarshalKey ( https://github.com/spf13/viper/blob/master/viper.go#L1582 )

[moorereason]

Is this fixed by https://github.com/spf13/viper/commit/9e56dacc08fbbf8c9ee2dbc717553c758ce42bc9 that was committed yesterday?

[gonejack]

I use viper in production and this do kill me.

[makew0rld]

Got bit by this today, using viper.Set. This really should be noted in the documentation. And then it should be fixed by using a sync.RWMutex everywhere the internal map is accessed.

[alexanderbez]

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.

[vykulakov]

IIRC the main problem was with reloading config on changes. So on reload, viper writes new option values to the internal map without any locks and does it in a separate goroutine. If anyone tries to read an option from this map (and this happens obviously in another goroutine) at the same time then we will get a concurrent access error. This problem cannot be solved with a kind of external wrapper with its own locks. As a result, this feature of viper is not completely usable.

[makew0rld]

IIRC the main problem was with reloading config on changes.

Doing concurrent Sets, or a Get* while Seting is also a problem, and one that is not documented.

[batazor]

I get this error periodically