Skip to content

Commit

Permalink
extkingpin: fix Content/Rewrite race (thanos-io#6870)
Browse files Browse the repository at this point in the history
Got this during `TestLimiter_StartConfigReloader`:

```
Read at 0x00c0005123f0 by goroutine 8711:
  github.com/thanos-io/thanos/pkg/extkingpin.(*staticPathContent).Content()
      /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:90 +0x28
  github.com/thanos-io/thanos/pkg/receive.ParseLimitConfigContent()
      /home/giedrius/dev/thanos/pkg/receive/limiter.go:203 +0x3e
  github.com/thanos-io/thanos/pkg/receive.(*Limiter).loadConfig()
      /home/giedrius/dev/thanos/pkg/receive/limiter.go:143 +0x65
  github.com/thanos-io/thanos/pkg/receive.(*Limiter).StartConfigReloader.func1()
      /home/giedrius/dev/thanos/pkg/receive/limiter.go:119 +0x207
  github.com/thanos-io/thanos/pkg/extkingpin.(*pollingEngine).start.func1()
      /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:64 +0x6b8
  github.com/thanos-io/thanos/pkg/extkingpin.(*pollingEngine).start.func2()
      /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:74 +0x56

Previous write at 0x00c0005123f0 by goroutine 8710:
  github.com/thanos-io/thanos/pkg/extkingpin.(*staticPathContent).Rewrite()
      /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:111 +0x529
  github.com/thanos-io/thanos/pkg/receive.TestLimiter_StartConfigReloader()
      /home/giedrius/dev/thanos/pkg/receive/limiter_test.go:44 +0x5aa
  testing.tRunner()
      /usr/lib/go-1.21/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /usr/lib/go-1.21/src/testing/testing.go:1648 +0x44
```

Fix it by protecting `t.content` with a mutex and by copying the content's
slice so that the caller wouldn't have access to the original slice.

Signed-off-by: Giedrius Statkevičius <[email protected]>
  • Loading branch information
GiedriusS authored and douglascamata committed Dec 13, 2023
1 parent 6051857 commit f654dbb
Showing 1 changed file with 15 additions and 4 deletions.
19 changes: 15 additions & 4 deletions pkg/extkingpin/path_content_reloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"crypto/sha256"
"os"
"path/filepath"
"sync"
"time"

"github.com/go-kit/log"
Expand Down Expand Up @@ -79,15 +80,22 @@ func (p *pollingEngine) start(ctx context.Context) error {
}

type staticPathContent struct {
content []byte
path string
contentMtx sync.Mutex
content []byte
path string
}

var _ fileContent = (*staticPathContent)(nil)

// Content returns the cached content.
func (t *staticPathContent) Content() ([]byte, error) {
return t.content, nil
t.contentMtx.Lock()
defer t.contentMtx.Unlock()

c := make([]byte, 0, len(t.content))
c = append(c, t.content...)

return c, nil
}

// Path returns the path to the file that contains the content.
Expand All @@ -102,12 +110,15 @@ func NewStaticPathContent(fromPath string) (*staticPathContent, error) {
if err != nil {
return nil, errors.Wrapf(err, "could not load test content: %s", fromPath)
}
return &staticPathContent{content, fromPath}, nil
return &staticPathContent{content: content, path: fromPath}, nil
}

// Rewrite rewrites the file backing this staticPathContent and swaps the local content cache. The file writing
// is needed to trigger the file system monitor.
func (t *staticPathContent) Rewrite(newContent []byte) error {
t.contentMtx.Lock()
defer t.contentMtx.Unlock()

t.content = newContent
// Write the file to ensure possible file watcher reloaders get triggered.
return os.WriteFile(t.path, newContent, 0666)
Expand Down

0 comments on commit f654dbb

Please sign in to comment.