x/sync/errgroup: why not recover the fn's err in errgroup

[henrycjchen]

What version of Go are you using (go version)?

$ go version
go version go1.13.6 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\henrycjchen\AppData\Local\go-build
set GOENV=C:\Users\henrycjchen\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\henrycjchen\go
set GOPRIVATE=
set GOPROXY=direct
set GOROOT=c:\go
set GOSUMDB=off
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\HENRYC~1\AppData\Local\Temp\go-build068436734=/tmp/go-build -gno-record-gcc-switches

What did you do?

we all know that use goroutine without a recover, the go program may cause a crash

What did you expect to see?

I expect that errgroup can recover the fn's error silently.

What is the reason not to do so? Is recover may decrease the performance of the program?

What did you see instead?

I should add defer func(){recover()}() inside the go func every time I use errgroup

[bcmills]

CL 134395 does propagate panics to the caller of Wait. It was a bit contentious, but I do plan to dust it off and see if I can push it forward someday.

[henrycjchen]

looking forward to it.

[ash2k]

I'd prefer the current behavior - if there is a panic, the program should crash.

Please note that the user can change the current behavior of this library using a tiny wrapper around it that recovers from panics, but if the library changes the behavior it'd be impossible to reverse it. Please let the user choose what their program should do.

[bcmills]

@ash2k, it is possible for a library to change the behavior either way.

Consider:

func unrecover(f func()) {
	defer func() {
		if r := recover(); r != nil {
			go panic(r)  // Move the panic to a new goroutine so that it cannot be recovered.
		}
	}()

	f()
}

[ash2k]

@bcmills Yes, but unrecover() loses the stacktrace, which is a big deal. https://play.golang.org/p/IVyAExGv6Vu

[hellodudu]

Perhaps we may make recover as an option when New?

defer func() {
	if g.recover {
		if err := recover(); err != nil {
			stack := string(debug.Stack())
			fmt.Println(stack)
		}
	}

	g.wg.Done()
}()

[mokiat]

I have a CLI tool that uses errgroup and ends with the following (or similar):

if err := group.Wait(); err != nil {
    log.Fatalf("error: %v", err)
}

The problem is that the last group (the slowest) was panic-ing, which would default to nil being returned from Wait. However, since the main goroutine would immediately exit, the panic would not be reported. Only after some troubleshooting of the code did I realize that something was panicing and that a race condition was occuring between the panic and the exiting of the app.

I have created a go playground demo for this:
https://go.dev/play/p/HtBT9LAN5c7

Run it multiple times (at least 5-10 times) to see that the outcome is inconsistent. The os.Exit was necessary here, since the demo is too simple and the two scenarios would be log+panic or log. Also, in this example the panic would be the more common scenario. I assume that my CLI just has more things going on which is the reason why the panic is a bit slower to propagate and rarely ever occurs.

[gopherbot]

Change https://go.dev/cl/416555 mentions this issue: errgroup: propagate panics and goexits from goroutines in the group

[bcmills]

I have filed #53757 as a concrete proposal to address panic behaviors in errgroup.Group.

Since this issue only raises the question of why the current API is the way it is, I am closing this issue as inactionable.
(The API is the way that it is only because there hasn't yet been a proposal accepted to change it. 😅)