Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extkingpin: fix Content/Rewrite race #6870

Merged
merged 1 commit into from
Nov 3, 2023
Merged

extkingpin: fix Content/Rewrite race #6870

merged 1 commit into from
Nov 3, 2023

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Nov 3, 2023

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.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

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]>
@GiedriusS GiedriusS merged commit 7e879c6 into main Nov 3, 2023
14 checks passed
@GiedriusS GiedriusS deleted the fix_extkingpin_race branch November 3, 2023 15:27
rabenhorst pushed a commit to rabenhorst/thanos that referenced this pull request Nov 15, 2023
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]>
Signed-off-by: Sebastian Rabenhorst <[email protected]>
douglascamata pushed a commit to douglascamata/thanos that referenced this pull request Dec 13, 2023
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]>
douglascamata pushed a commit to douglascamata/thanos that referenced this pull request Dec 13, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants