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.

Sign up for GitHub

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

Jump to bottom

golang.org/x/sync/errgroup: strange data race #65045

Closed
dkropachev opened this issue Jan 9, 2024 · 4 comments
Closed

golang.org/x/sync/errgroup: strange data race #65045

dkropachev opened this issue Jan 9, 2024 · 4 comments

Comments

@dkropachev
Copy link

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/dmitry.kropachev/.cache/go-build'
GOENV='/home/dmitry.kropachev/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/dmitry.kropachev/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/dmitry.kropachev/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/extra/scylladb/go/go1.21.6'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/extra/scylladb/go/go1.21.6/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/dmitry.kropachev/GolandProjects/awesomeProject/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build367611483=/tmp/go-build -gno-record-gcc-switches'

What did you do?

data_race_test.go:

import (
	"sync"
	"testing"

	"github.com/pkg/errors"
	"golang.org/x/sync/errgroup"
)

func TestPanic(t *testing.T) {
	wg := sync.WaitGroup{}
	var err error

	testBody := func() error {
		return errors.New("some error")
	}

	errGrp := errgroup.Group{}

	for x := 0; x < 100; x++ {
		wg.Add(1)
		errGrp.Go(func() error {
			err = testBody()
			return nil
		})
	}
	t.Log(err)
	t.Log(errGrp.Wait())
}

func TestNoPanic(t *testing.T) {
	wg := sync.WaitGroup{}
	var err error

	testBody := func() error {
		return errors.New("some error")
	}

	for x := 0; x < 100; x++ {
		wg.Add(1)
		func() {
			err = testBody()
		}()
	}
	t.Log(err)
}

What did you see happen?

Rungo test -v -race data_race_test.go -run TestPanic:

=== RUN   TestPanic
==================
WARNING: DATA RACE
Write at 0x00c0001a0230 by goroutine 9:
  command-line-arguments_test.TestPanic.func2()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:24 +0x4b
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:75 +0x76

Previous write at 0x00c0001a0230 by goroutine 12:
  command-line-arguments_test.TestPanic.func2()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:24 +0x4b
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:75 +0x76

Goroutine 9 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:72 +0x124
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:23 +0xfe
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44

Goroutine 12 (finished) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:72 +0x124
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:23 +0xfe
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44
==================
==================
WARNING: DATA RACE
Read at 0x00c0001a0230 by goroutine 7:
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:28 +0x1dd
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44

Previous write at 0x00c0001a0230 by goroutine 105:
  command-line-arguments_test.TestPanic.func2()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:24 +0x4b
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:75 +0x76

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x82a
  testing.runTests.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.runTests()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:2052 +0x896
  testing.(*M).Run()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1925 +0xb57
  main.main()
      _testmain.go:49 +0x2bd

Goroutine 105 (finished) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:72 +0x124
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:23 +0xfe
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44
==================
==================
WARNING: DATA RACE
Read at 0x00c000412108 by goroutine 7:
  github.com/pkg/errors.(*fundamental).Format()
      /home/dmitry.kropachev/go/pkg/mod/github.com/pkg/errors@v0.9.1/errors.go:137 +0x98
  fmt.(*pp).handleMethods()
      /extra/scylladb/go/go1.21.0/src/fmt/print.go:640 +0x1f0
  fmt.(*pp).printArg()
      /extra/scylladb/go/go1.21.0/src/fmt/print.go:756 +0xcce
  fmt.(*pp).doPrintln()
      /extra/scylladb/go/go1.21.0/src/fmt/print.go:1223 +0x4b
  fmt.Sprintln()
      /extra/scylladb/go/go1.21.0/src/fmt/print.go:321 +0x48
  testing.(*common).Log()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1045 +0x68
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:28 +0x24c
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44

Previous write at 0x00c000412108 by goroutine 104:
  github.com/pkg/errors.Errorf()
      /home/dmitry.kropachev/go/pkg/mod/github.com/pkg/errors@v0.9.1/errors.go:114 +0x84
  command-line-arguments_test.TestPanic.func1()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:16 +0x53
  command-line-arguments_test.TestPanic.func2()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:24 +0x37
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:75 +0x76

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x82a
  testing.runTests.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.runTests()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:2052 +0x896
  testing.(*M).Run()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1925 +0xb57
  main.main()
      _testmain.go:49 +0x2bd

Goroutine 104 (finished) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/dmitry.kropachev/go/pkg/mod/golang.org/x/sync@v0.0.0-20220722155255-886fb9371eb4/errgroup/errgroup.go:72 +0x124
  command-line-arguments_test.TestPanic()
      /home/dmitry.kropachev/GolandProjects/awesomeProject/data_race_test.go:23 +0xfe
  testing.tRunner()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /extra/scylladb/go/go1.21.0/src/testing/testing.go:1648 +0x44
==================
    data_race_test.go:28: some error: 10
    data_race_test.go:29: <nil>
    testing.go:1465: race detected during execution of test
--- FAIL: TestPanic (0.00s)
=== NAME  
    testing.go:1465: race detected during execution of test
FAIL
FAIL    command-line-arguments  0.011s
FAIL

Rungo test -v -race data_race_test.go -run TestNoPanic:

=== RUN   TestNoPanic
    data_race_test.go:46: some error
--- PASS: TestNoPanic (0.00s)
PASS
ok      command-line-arguments  1.009s

What did you expect to see?

Rungo test -v -race data_race_test.go -run TestPanic:

=== RUN   TestPanic
    data_race_test.go:28: some error: 10
    data_race_test.go:29: <nil>
--- PASS: TestPanic (0.00s)
PASS
ok      command-line-arguments  0.002s

Rungo test -v -race data_race_test.go -run TestNoPanic:

=== RUN   TestNoPanic
    data_race_test.go:46: some error
--- PASS: TestNoPanic (0.00s)
PASS
ok      command-line-arguments  1.009s
@Jorropo
Copy link
Member

Jorropo commented Jan 9, 2024

Your err exists outside of the closure and is captured by the closure, errgroup.Go then starts a goroutine, then all the goroutines try to write to the same captured variable.

Race is correct here, I don't get why this is supposed to work.

@Jorropo
Copy link
Member

Jorropo commented Jan 9, 2024 •
edited

A minified version of your code is this:

var wg sync.WaitGroup
const n = 100
wg.Add(n)
var sink int

for range n {
 go func() {
  defer wg.Done()
  sink = 42
 }()
}

wg.Wait()
return sink

@bcmills
Copy link
Contributor

bcmills commented Jan 9, 2024

Agreed — the race detector is correctly describing a race in the user code. (And the race has ~nothing to do with errgroup.)

@bcmills bcmills closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
@dkropachev
Copy link
Author

@bcmills , @Jorropo , thanks for quick response and sorry for spam, I see my mystake now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bcmills @Jorropo @dkropachev