Skip to content

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

Open
@fmoor

Description

@fmoor

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

Activity

added
BugReportIssues describing a possible bug in the Go implementation.
on Apr 3, 2025
seankhliao

seankhliao commented on Apr 4, 2025

@seankhliao
Member

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.

adonovan

adonovan commented on Apr 4, 2025

@adonovan
Member

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

added
NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.
on Apr 4, 2025
bcmills

bcmills commented on Apr 5, 2025

@bcmills
Contributor

but not if panicnil=0.

For that you could use a double defer sandwich!

gopherbot

gopherbot commented on Apr 5, 2025

@gopherbot
Contributor

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

qiulaidongfeng

qiulaidongfeng commented on Apr 5, 2025

@qiulaidongfeng
Member

but not if panicnil=0.

For that you could use a double defer sandwich!

See CL 662816 , this breaks the stack trace.

aclements

aclements commented on Apr 6, 2025

@aclements
Member

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:

  1. It always panics (more or less the current behavior, though we can make the panic nicer)
  2. The first call does a Goexit, and any later calls panic with some helpful message
  3. 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.

fmoor

fmoor commented on Apr 6, 2025

@fmoor
Author

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
	})
}
adonovan

adonovan commented on Apr 6, 2025

@adonovan
Member

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.

added this to the Go1.25 milestone on Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugReportIssues describing a possible bug in the Go implementation.NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.compiler/runtimeIssues related to the Go compiler and/or runtime.

    Type

    No type

    Projects

    Status

    Todo

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @cagedmantis@mknyszek@aclements@bcmills@adonovan

        Issue actions

          sync: Once panics if testing.T.FailX or testing.T.SkipX are called · Issue #73159 · golang/go