Skip to content

sg: fix data race in Runner by wrapping access to progress in mutex

Warren Gifford requested to merge mrn/sg-fix-data-race into main

Created by: mrnugget

My build failed because of a data race in TestRunnerInteractive

START| RunnerInteractive
START|   RunnerInteractive/bad_input[0m
WARNING: DATA RACE
Write at 0x00c0001a5d88 by goroutine 93:
  github.com/sourcegraph/sourcegraph/lib/output.(*progressSimple).SetValue()
      /root/buildkite/build/sourcegraph/lib/output/progress_simple.go:32 +0xb3
  github.com/sourcegraph/sourcegraph/lib/output.(*progressWithStatusBarsSimple).SetValue()
      <autogenerated>:1 +0x29
  github.com/sourcegraph/sourcegraph/dev/sg/internal/check.(*Runner[...]).runAllCategoryChecks.func1()
      /root/buildkite/build/sourcegraph/dev/sg/internal/check/runner.go:195 +0x98
      [...]
Previous write at 0x00c0001a5d88 by goroutine 90:
  github.com/sourcegraph/sourcegraph/lib/output.(*progressSimple).SetValue()
      /root/buildkite/build/sourcegraph/lib/output/progress_simple.go:32 +0xb3
  github.com/sourcegraph/sourcegraph/lib/output.(*progressWithStatusBarsSimple).SetValue()
      <autogenerated>:1 +0x29
  github.com/sourcegraph/sourcegraph/dev/sg/internal/check.(*Runner[...]).runAllCategoryChecks.func1()
      /root/buildkite/build/sourcegraph/dev/sg/internal/check/runner.go:195 +0x98
      [...]
Goroutine 93 (running) created at:
  github.com/sourcegraph/sourcegraph/lib/group.(*orderedStreamer[...]).initOnce.func1()
      /root/buildkite/build/sourcegraph/lib/group/stream.go:274 +0xc4
  sync.(*Once).doSlow()
      /root/.asdf/installs/golang/1.18.1/go/src/sync/once.go:68 +0x101
      [...]
Goroutine 90 (running) created at:
  github.com/sourcegraph/sourcegraph/lib/group.(*orderedStreamer[...]).initOnce.func1()
      /root/buildkite/build/sourcegraph/lib/group/stream.go:274 +0xc4
  sync.(*Once).doSlow()
      /root/.asdf/installs/golang/1.18.1/go/src/sync/once.go:68 +0x101
      [...]

Updating the checks counter was safe, since it used atomic.Float64, but multiple writes to progress lead to the data race. So this changes the code to move all the global-state-mutating actions into separate functions that all use a mutex. That way we don't need atomic anymore and writes to progress are protected.

Test plan

  • Existing tests

Merge request reports