Golang sync: Once panics if testing.T.FailX or testing.T.SkipX are called

Go version

go version go1.24.2 linux/amd64

Output of `go env` in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/fmoor/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/fmoor/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3782144130=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/fmoor/src/proj/go.mod'
GOMODCACHE='/home/fmoor/.go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/fmoor/.go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/fmoor/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.2'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

package main

import (
        "sync"
        "testing"
)

func Setup(t *testing.T) func() {
        return sync.OnceFunc(func() {
                t.SkipNow()
        })
}

func TestA(t *testing.T) {
        future := Setup(t)
        future()
}

What did you see happen?

$ go test -v
=== RUN   Test
--- FAIL: Test (0.00s)
panic: panic called with nil argument [recovered]
        panic: panic called with nil argument

goroutine 18 [running]:
testing.tRunner.func1.2({0x550e20, 0x6c98a0})
        /usr/local/go/src/testing/testing.go:1734 +0x21c
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1737 +0x35e
panic({0x0?, 0x0?})
        /usr/local/go/src/runtime/panic.go:792 +0x132
example%2ecom.Test.Setup.OnceFunc.func2.1()
        /usr/local/go/src/sync/oncefunc.go:24 +0x69
runtime.Goexit()
        /usr/local/go/src/runtime/panic.go:636 +0x5e
testing.(*common).SkipNow(0xc000102700)
        /usr/local/go/src/testing/testing.go:1156 +0x45
example%2ecom.Test.Setup.func1()
        /home/fmoor/src/proj/example_test.go:10 +0x19
example%2ecom.Test.Setup.OnceFunc.func2()
        /usr/local/go/src/sync/oncefunc.go:27 +0x62
sync.(*Once).doSlow(0x66b?, 0x66a?)
        /usr/local/go/src/sync/once.go:78 +0xab
sync.(*Once).Do(...)
        /usr/local/go/src/sync/once.go:69
example%2ecom.Test.Setup.OnceFunc.func3(...)
        /usr/local/go/src/sync/oncefunc.go:32
example%2ecom.Test(0xc000102700?)
        /home/fmoor/src/proj/example_test.go:16 +0x85
testing.tRunner(0xc000102700, 0x5842a0)
        /usr/local/go/src/testing/testing.go:1792 +0xf4
created by testing.(*T).Run in goroutine 1
        /usr/local/go/src/testing/testing.go:1851 +0x413
exit status 2
FAIL    example.com     0.004s

What did you expect to see?

sync.Once seems to incorrectly infer a panic when runtime.Goexit is called. I would expect the test to be skipped instead of panic.

$ go test -v
=== RUN   Test
--- SKIP: Test (0.00s)
PASS
ok      example.com     0.001s

Comment From: gabyhelp

Related Issues

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

Comment From: seankhliao

Looking at the implementation https://cs.opensource.google/go/go/+/refs/tags/go1.24.2:src/sync/oncefunc.go;l=11-37 I think it can be fixed for the default case (just check for non nil recovered value), but not if panicnil=0.

Comment From: adonovan

I agree. Just today we are making a similar change in errgroup; see https://go.dev/cl/644575.

Comment From: bcmills

but not if panicnil=0.

For that you could use a double defer sandwich!

Comment From: gopherbot

Change https://go.dev/cl/662816 mentions this issue: sync: Once does not panic when Goexit is called

Comment From: qiulaidongfeng

but not if panicnil=0.

For that you could use a double defer sandwich!

See CL 662816 , this breaks the stack trace.

Comment From: aclements

Maybe we should do something differently on a Goexit, but I don't understand why it makes sense to FailX or SkipX inside a OnceFunc. Since this OnceFunc closes over t, it's only ever meaningful to call it again from the same test, but you can never do that because it just exited that test.

Is the idea is that, if the OnceFunc exits the test, you'll never call it again, but if it returns normally you may call it more times within the same test?

What should happen if a OnceFunc does a Goexit on the first call and then you call it again? I can see three possibilities:

It always panics (more or less the current behavior, though we can make the panic nicer)
The first call does a Goexit, and any later calls panic with some helpful message
The first and subsequent calls do a Goexit.

I don't like option 3 because usually Goexit is preceded by somehow signaling why this goroutine is exiting, and subsequent calls wouldn't do that. For example, this happens in testing.T.SkipNow. In some sense, the behavior the OnceFunc is capturing should be "Goexit a particular goroutine" and it can't repeat that behavior on another goroutine.

I'd be okay with options 1 or 2.

Comment From: fmoor

My use case was to do expensive test fixture setup in another goroutine and return the fixture to the test using the function returned from sync.OnceValue. The goal is to allow for expensive fixtures to be created concurrently. The sync.OnceValue function also has to handle the case where the fixture setup failed. In that case it calls t.FailNow.

func DBFixture(t *testing.T) func() *sql.DB {
    connChan := make(chan *sql.DB, 1)
    go func() {
        // start a database and migrate it
        connChan <- startDatabase()
    }()

    return sync.OnceValue(func() *sql.DB {
        conn := <-connChan
        if conn == nil {
            t.Fatalf("database setup failed")
        }
        return conn
    })
}

Comment From: adonovan

I don't understand why it makes sense to FailX or SkipX inside a OnceFunc.

It seems reasonable to me. Imagine your test passes some lambda into a library that calls it within a sync.Once to memoize an expensive computation. Now imagine that sometimes the lambda calls Fatal when it can't complete its task. The sync.Once is just a detail of the library. (A pedant might argue that the test shouldn't assume that the lambda will be called from the same goroutine, and that means it's not morally permitted to call Fatal--I am that pedant!--but in fact tests do this all the time.)

Is the idea is that, if the OnceFunc exits the test, you'll never call it again, but if it returns normally you may call it more times within the same test?

The idea is that the OnceFunc is a temporary variable created by the test. Nothing says a Once needs to have global extent.

What should happen if a OnceFunc does a Goexit on the first call and then you call it again? I can see three possibilities:

I think it can't be option 1, because that would cause the testing package to report that the test panicked even though it actually just called t.Fatal from within the Once. And it shouldn't be option 3 for the reason you gave. So that leaves option 2. Once.Do calls subsequent to a Goexit should never be allowed to happen in the scenario I imagined above; making them panic would at least give the user an informative error message explaining that they are misusing Once.