nil-go / konf

The simplest config loader for Go that reads/watches from file, env, flag and clouds (AWS, Azure, GCP).
https://pkg.go.dev/github.com/nil-go/konf
MIT License
238 stars 9 forks source link

Fixed data race #377

Closed BoskyWSMFN closed 4 months ago

BoskyWSMFN commented 4 months ago

Fixed data race occurred in *Config.values, *Config.providers and provider.values. go test log:

WARNING: DATA RACE
Write at 0x00c0001af9a8 by goroutine 34:
  github.com/nil-go/konf.(*Config).Watch.func3.1()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:108 +0x116
  github.com/nil-go/konf/provider/file.(*File).Watch()
      /go/pkg/mod/github.com/nil-go/konf/provider/file@v1.2.0/watch.go:84 +0xb4c
  github.com/nil-go/konf.(*Config).Watch.func3()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:135 +0x3e6
  github.com/nil-go/konf.(*Config).Watch.gowrap3()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:138 +0x4f
Previous read at 0x00c0001af9a8 by goroutine 32:
  github.com/nil-go/konf.(*Config).Watch.func2()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:56 +0x2fa
Goroutine 34 (running) created at:
  github.com/nil-go/konf.(*Config).Watch()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:101 +0x88c
WARNING: DATA RACE
Write at 0x00c0003302d0 by goroutine 32:
  github.com/nil-go/konf.(*Config).Watch.func2()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/watch.go:59 +0x207
Previous read at 0x00c0003302d0 by goroutine 43:
  github.com/nil-go/konf.(*Config).Unmarshal()
      /go/pkg/mod/github.com/nil-go/konf@v1.2.1-0.20240613001052-a88c18635dda/config.go:133 +0x146
codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.0%. Comparing base (a88c186) to head (de49d7b). Report is 6 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/nil-go/konf/pull/377/graphs/tree.svg?width=650&height=150&src=pr&token=2LIUXHDH9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nil-go)](https://app.codecov.io/gh/nil-go/konf/pull/377?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nil-go) ```diff @@ Coverage Diff @@ ## main #377 +/- ## ======================================= + Coverage 82.8% 83.0% +0.1% ======================================= Files 62 62 Lines 2654 2684 +30 ======================================= + Hits 2200 2230 +30 Misses 371 371 Partials 83 83 ``` | [Files](https://app.codecov.io/gh/nil-go/konf/pull/377?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nil-go) | Coverage Δ | | |---|---|---| | [config.go](https://app.codecov.io/gh/nil-go/konf/pull/377?src=pr&el=tree&filepath=config.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nil-go#diff-Y29uZmlnLmdv) | `100.0% <100.0%> (ø)` | | | [watch.go](https://app.codecov.io/gh/nil-go/konf/pull/377?src=pr&el=tree&filepath=watch.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nil-go#diff-d2F0Y2guZ28=) | `100.0% <100.0%> (ø)` | |
ktong commented 4 months ago

Thanks for catching the race. Lock is easy but heavy solution. Please give me time to think about a light alternative.

ktong commented 4 months ago

By the way, could you share your test? I run go test -v -shuffle=on -count=100 -race ./... under konf and it always succeed on my Mac

ktong commented 4 months ago

378 is an alternative which using atomic.Pointer. I added the test to verify it works.

Although simple pointer also works since konf allows near latest value but it's annoying since it always fails race test.

BoskyWSMFN commented 4 months ago

378 is an light alternative. I need to add tests on it

Ah, sure. I had an idea to use sync.Map but this solution could broke something related to config or provider structs.

BoskyWSMFN commented 4 months ago

Thanks for catching the race. Lock is easy but heavy solution. Please give me time to think about a light alternative.

Just a brute force solution. Atomic pointer looks way better. I am quite short on time to dig into the code so doing what I can trying not to break something.

ktong commented 4 months ago

then I close this PR and use #378. Would you please take a look at #378? thanks

ktong commented 4 months ago

Thanks for catching the race. Lock is easy but heavy solution. Please give me time to think about a light alternative.

Just a brute force solution. Atomic pointer looks way better. I am quite short on time to dig into the code so doing what I can trying not to break something.

Cool. I just merged the PR. Please let me know if you would like a new release.

BoskyWSMFN commented 4 months ago

go test -v -shuffle=on -count=100 -race

Caught this race in a docker gitlab runner so i can't do it there. No data races on my linux machine though.

Command:

CGO_ENABLED=1 GOARCH=amd64 CC='gcc' CXX='g++' CGO_CFLAGS='-g1 -O2' CGO_CPPFLAGS='' CGO_LDFLAGS='-static-libgcc -static-libstdc++ -static ' go test -timeout 5m -race -short -tags 'netgo' -ldflags '-linkmode external'  `go list ./... | grep -v external `  # except external dir

Code where data race appeared:

func (c *configManagement) setOnChangeHook(ctx context.Context) {
    c.Konf().OnChange(c.unmarshalConfigsLogError(ctx))
}

func (c *configManagement) unmarshalConfigsLogError(ctx context.Context) func(*konf.Config) {
    return func(konfCfg *konf.Config) {
        go func() {
            // some code...

            err := c.unmarshalConfigs(konfCfg)
            if err != nil {
                c.logger.ErrorContext(ctx, "c.unmarshalConfigs():", slogAttrError(err))
            }

            // some code...
        }()
    }
}

I believe the problem was in this new routine in the hook.

ktong commented 4 months ago

I believe it has been fixed in #378