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

fix: Add data race protection for the tm.wg var #12100

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

paul1r
Copy link
Contributor

@paul1r paul1r commented Feb 29, 2024

What this PR does / why we need it:
Moves access to the table_manager.wg var behind the existing tablesMtx mutex
Which issue(s) this PR fixes:
relates to: #8586

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@paul1r paul1r requested a review from a team as a code owner February 29, 2024 22:08
@paul1r
Copy link
Contributor Author

paul1r commented Feb 29, 2024

Previously:

==================
WARNING: DATA RACE
Write at 0x00c0005cb2a0 by goroutine 163:
  runtime.racewrite()
      <autogenerated>:1 +0x10
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.(*tableManager).Stop()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager.go:154 +0x60
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.buildTestTableManager.func2()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager_test.go:68 +0x3c
  runtime.deferreturn()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/runtime/panic.go:477 +0x34
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1648 +0x40

Previous read at 0x00c0005cb2a0 by goroutine 164:
  runtime.raceread()
      <autogenerated>:1 +0x10
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.(*tableManager).loop()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager.go:119 +0x40
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.NewTableManager.func1()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager.go:114 +0x34

Goroutine 163 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1648 +0x5e8
  testing.runTests.func1()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:2054 +0x80
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1595 +0x1b0
  testing.runTests()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:2052 +0x6e4
  testing.(*M).Run()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1925 +0x9ec
  main.main()
      _testmain.go:69 +0x294

Goroutine 164 (finished) created at:
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.NewTableManager()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager.go:114 +0x404
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.buildTestTableManager()
      
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager_test.go:62 +0x2fc
  github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads.TestTableManager_cleanupCache()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads/table_manager_test.go:103 +0x44
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1595 +0x1b0
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1648 +0x40
==================
    testing.go:1465: race detected during execution of test
--- FAIL: TestTableManager_cleanupCache (0.00s)

@paul1r paul1r changed the title bug: Add protection for the tm.wg var fix: Add data race protection for the tm.wg var Feb 29, 2024
@paul1r
Copy link
Contributor Author

paul1r commented Feb 29, 2024

After fix:

go test -race -count=1000  . -run TestTableManager_cleanupCache
ok  	github.com/grafana/loki/pkg/storage/stores/shipper/indexshipper/downloads	1.916s

@paul1r paul1r merged commit 77a2580 into main Feb 29, 2024
13 checks passed
@paul1r paul1r deleted the paul1r/tablemanager_data_race branch February 29, 2024 22:47
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
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.

None yet

2 participants