x/tools/gopls/internal/analysis/modernize: bloop may cause deadlock when used inside goroutines in benchmarks

[cuishuang]

gopls version

binary compiled by latest code

go env

AR='ar'
CC='cc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='c++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN='/Users/mac/go/bin'
GOCACHE='/Users/mac/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/mac/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/tq/m955dwkd1519phkp2hwpv9_80000gn/T/go-build2400307511=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/Users/mac/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/mac/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/mac/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.3'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

For the following code:

package main

import (
	"sync"
	"testing"
)

type dummyEvent struct{}

type dummyMux struct{}

func (m *dummyMux) Post(ev dummyEvent) {}

var mux = new(dummyMux)

func BenchmarkNInGoroutine(b *testing.B) {
	var wg sync.WaitGroup
	poster := func() {
		for i := 0; i < b.N; i++ {
			mux.Post(dummyEvent{})
		}
		wg.Done()
	}
	wg.Add(2)
	for i := 0; i < 2; i++ {
		go poster()
	}
	wg.Wait()
}

After being processed by the modernize analyzer, it will be transformed into the following

package main

import (
	"sync"
	"testing"
)

type dummyEvent struct{}

type dummyMux struct{}

func (m *dummyMux) Post(ev dummyEvent) {}

var mux = new(dummyMux)

func BenchmarkNInGoroutine(b *testing.B) {
	var wg sync.WaitGroup
	poster := func() {
		for b.Loop() {
			mux.Post(dummyEvent{})
		}
		wg.Done()
	}
	wg.Add(2)
	for i := 0; i < 2; i++ {
		go poster()
	}
	wg.Wait()
}

This may lead to a deadlock.

What did you see happen?

go test -run=^$ -bench=. -timeout=1h

fatal error: all goroutines are asleep - deadlock!

goroutine 1 [chan receive, 3 minutes]:
testing.(*B).run1(0x140001422c8)
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:247 +0xa0
testing.(*B).Run(0x14000142008, {0x102957d2a?, 0x1400007aae8?}, 0x1029ce6c8)
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:847 +0x3f0
testing.runBenchmarks.func1(0x14000142008)
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:708 +0x40
testing.(*B).runN(0x14000142008, 0x1)
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:219 +0x1a4
testing.runBenchmarks({0x102956a87, 0x14}, 0x0?, {0x102aaf020, 0x2, 0x7?})
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:717 +0x51c
testing.(*M).Run(0x1400007e140)
        /opt/homebrew/opt/go/libexec/src/testing/testing.go:2158 +0x86c
main.main()
        _testmain.go:47 +0x90

goroutine 20 [sync.WaitGroup.Wait, 3 minutes]:
sync.runtime_SemacquireWaitGroup(0x140000828c0?)
        /opt/homebrew/opt/go/libexec/src/runtime/sema.go:110 +0x2c
sync.(*WaitGroup).Wait(0x1400000e1a0)
        /opt/homebrew/opt/go/libexec/src/sync/waitgroup.go:118 +0x70
demo/modernize/bloop.BenchmarkLoopInGoroutine(0x140001422c8)
        /Users/mac/find_tools_bug/modernize/bloop/event_test.go:29 +0xb4
testing.(*B).runN(0x140001422c8, 0x1)
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:219 +0x1a4
testing.(*B).run1.func1()
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:245 +0x4c
created by testing.(*B).run1 in goroutine 1
        /opt/homebrew/opt/go/libexec/src/testing/benchmark.go:238 +0x90
exit status 2
FAIL    demo/modernize/bloop    183.950s

What did you expect to see?

b.Loop() must only be used in the benchmark’s main goroutine, not in worker goroutines.

So the modernize analyzer should ignore this case.

Editor and settings

No response

Logs

No response

[gopherbot]

Change https://go.dev/cl/706675 mentions this issue: gopls/internal/analysis/modernize: fix the potential deadlock issues in bloop

[gabyhelp]

Related Issues

• x/tools/gopls: modernize bloop: Can't use b.Loop() if loop contains b.StopTimer/b.StartTimer #74967
• x/tools/gopls/internal/analysis/modernize: produces invalid go code #75277 (closed)
• testing: inconsistent behaviors between running tests directly and after compiling the code first #59879 (closed)
• runtime: some kind deadlock issue with the new scheduler #4998 (closed)
• testing: Benchmark calls during benchmarks deadlock #29727
• runtime: malloc instancing generic function seems to deadlock(ish) during benchmark #58744 (closed)
• testing/synctest: Repeated sync.WaitGroup.Add appears flaky under synctest #74386 (closed)
• cmd/vendor/golang.org/x/arch/ppc64/ppc64asm: test deadlock without -short #17698 (closed)
• goroutine preemption failed #34136 (closed)
• cmd/go: tests timing out on linux-amd64 #21850 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

Here's an example that exhibits the problem without either concurrency or anonymous functions:

func BenchmarkFoo(b *testing.B) {
	for i := 0; i < b.N; i++ {
		foo()
	}
	for i := 0; i < b.N; i++ {
		bar()
	}
}

func foo() { time.Sleep(10 * time.Millisecond) }
func bar() { time.Sleep(10 * time.Millisecond) }

Before: 20ms per op
After: error: B.Loop called with timer stopped

I think the criterion needs to something like: only modernize b.N loops when they appear directly within the Benchmark function and control passes through them exactly once.

[cuishuang]

It seems that's indeed the case... the reason is not the goroutines, but the multiple calls to b.Loop().

[cuishuang]

I'm not sure if others have any good suggestions? If not, I'll go ahead and make this change.