Skip to content

RunSingleCommand race condition in waitForInterrupts

Alessio Caiazza requested to merge nolith/gitlab-runner:bugfix/single_runner_race_condition into master

What does this MR do?

This patch fixes a race condition on the shared RunSingleCommand.finished

Why was this MR needed?

Accessing concurrently bool isn't thread safe, the runner could crash during a graceful shutdown.

$ go test ./commands -race -run TestSingleRunner
time="2017-06-02T17:27:37+02:00" level=info msg="Starting runner for http://example.com with token _test_to ..."
==================
WARNING: DATA RACE
Write at 0x00c420398778 by goroutine 12:
  gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.waitForInterrupts()
      /Users/nolith/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/single.go:32 +0x8f5

Previous read at 0x00c420398778 by goroutine 11:
  gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.(*RunSingleCommand).Execute()
      /Users/nolith/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/single.go:147 +0x621
  gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.TestSingleRunnerSigquit()
      /Users/nolith/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/single_test.go:65 +0x1538
  testing.tRunner()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:610 +0xc9

Goroutine 12 (running) created at:
  gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.(*RunSingleCommand).Execute()
      /Users/nolith/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/single.go:143 +0x58e
  gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.TestSingleRunnerSigquit()
      /Users/nolith/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/single_test.go:65 +0x1538
  testing.tRunner()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:610 +0xc9

Goroutine 11 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:646 +0x52f
  testing.RunTests.func1()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:793 +0xb9
  testing.tRunner()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:610 +0xc9
  testing.RunTests()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:799 +0x4ea
  testing.(*M).Run()
      /usr/local/Cellar/go@1.7/1.7.5/libexec/src/testing/testing.go:743 +0x12f
  main.main()
      gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/_test/_testmain.go:74 +0x1b8
==================
time="2017-06-02T17:27:37+02:00" level=warning msg="Requested quit, waiting for builds to finish"
time="2017-06-02T17:27:38+02:00" level=info msg="Starting runner for http://example.com with token _test_to ..."
time="2017-06-02T17:27:38+02:00" level=info msg="This runner has processed its build limit, so now exiting"
PASS
Found 1 data race(s)
FAIL	gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands	2.101s

Are there points in the code the reviewer needs to double check?

The patch itself is quite small, I didn't want to introduce too many changes in RunSingleCommand so I added github.com/tevino/abool which is a very simple implementation of atomic bool based on sync/atomic in less than 70 lines.

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Tests
    • Added for this feature/bug
    • All builds are passing
  • Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

None

Merge request reports