Skip to content

Commit

Permalink
Cleanup data race associated with workerID var (grafana#11922)
Browse files Browse the repository at this point in the history
**What this PR does / why we need it**:
A data race existed with the workerID variable, as it could be modified
by multiple goroutines.

Relates to: grafana#8586
--

Before fix:
```
go test -count=1 -race ./pkg/querier/worker
==================
WARNING: DATA RACE
Read at 0x00c000494108 by goroutine 229:
  github.com/grafana/loki/pkg/querier/worker.(*processorManager).concurrency.func1()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/processor_manager.go:81 +0x118

Previous write at 0x00c000494108 by goroutine 222:
  github.com/grafana/loki/pkg/querier/worker.(*processorManager).concurrency()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/processor_manager.go:70 +0x108
  github.com/grafana/loki/pkg/querier/worker.(*querierWorker).resetConcurrency()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker.go:267 +0x10c
  github.com/grafana/loki/pkg/querier/worker.(*querierWorker).AddressAdded()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker.go:219 +0x868
  github.com/grafana/loki/pkg/querier/worker.TestResetConcurrency.func1()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker_test.go:64 +0x1c8
  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

Goroutine 229 (running) created at:
  github.com/grafana/loki/pkg/querier/worker.(*processorManager).concurrency()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/processor_manager.go:75 +0xcc
  github.com/grafana/loki/pkg/querier/worker.(*querierWorker).resetConcurrency()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker.go:267 +0x10c
  github.com/grafana/loki/pkg/querier/worker.(*querierWorker).AddressAdded()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker.go:219 +0x868
  github.com/grafana/loki/pkg/querier/worker.TestResetConcurrency.func1()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker_test.go:64 +0x1c8
  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

Goroutine 222 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/testing/testing.go:1648 +0x5e8
  github.com/grafana/loki/pkg/querier/worker.TestResetConcurrency()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/querier/worker/worker_test.go:52 +0x1b0
  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
==================
--- FAIL: TestResetConcurrency (0.02s)
    --- FAIL: TestResetConcurrency/concurrency_is_correct_when_numTargets_does_not_divide_evenly_into_maxConcurrent (0.01s)
        testing.go:1465: race detected during execution of test
    testing.go:1465: race detected during execution of test
FAIL
FAIL	github.com/grafana/loki/pkg/querier/worker	4.626s
FAIL
```

--

After fix:

```
go clean -testcache
go test -count=1 -race ./pkg/querier/worker
ok  	github.com/grafana/loki/pkg/querier/worker	6.034s
```
**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/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](grafana@d10549e)
- [ ] 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](grafana@0d4416a)
  • Loading branch information
paul1r authored and rhnasc committed Apr 12, 2024
1 parent ead5963 commit 4985985
Showing 1 changed file with 3 additions and 4 deletions.
7 changes: 3 additions & 4 deletions pkg/querier/worker/processor_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,20 @@ func (pm *processorManager) concurrency(n int) {
n = 0
}

workerID := 0
for len(pm.cancels) < n {
workerID++
workerID := len(pm.cancels) + 1
ctx, cancel := context.WithCancel(pm.ctx)
pm.cancels = append(pm.cancels, cancel)

pm.wg.Add(1)
go func() {
go func(workerID int) {
defer pm.wg.Done()

pm.currentProcessors.Inc()
defer pm.currentProcessors.Dec()

pm.p.processQueriesOnSingleStream(ctx, pm.conn, pm.address, strconv.Itoa(workerID))
}()
}(workerID)
}

for len(pm.cancels) > n {
Expand Down

0 comments on commit 4985985

Please sign in to comment.