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

x/sync/singleflight: panic in Do fn results in deadlock #33519

Closed
xonstone opened this issue Aug 7, 2019 · 11 comments
Closed

x/sync/singleflight: panic in Do fn results in deadlock #33519

xonstone opened this issue Aug 7, 2019 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@xonstone
Copy link

xonstone commented Aug 7, 2019 •
edited

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

go version go1.12.7 darwin/amd64

Does this issue reproduce with the latest release?

This is the newest stable version

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/user/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/user/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.7/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lq/0sdfc0ps1j5921_9zyy2cmz80000gn/T/go-build771037982=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The fn given to the singleflight group.Do panicked and was recovered outside the group.Do.

https://play.golang.org/p/s7UKLR6JqRt

What did you expect to see?

The call would be discarded and the lock released.(Maybe the panic would be passed to all waiting calls with the same key)

What did you see instead?

The call panicked but the lock was never released resulting in a deadlock of all following calls with the same key.
@gopherbot gopherbot added this to the Unreleased milestone Aug 7, 2019
@av86743
Copy link

av86743 commented Aug 7, 2019

panic occurs upon invocation of fn() here . Unfortunately API of DoChan/Result does not provide means to return/propagate panic.

@beoran
Copy link

beoran commented Aug 8, 2019

The simplest workaround is to defer a recover inside the callback function that assigns it's value to a variable in the outer scope, like this:

https://play.golang.org/p/YjaqG4vhDMR

Hope this is of any huse to you.

@xonstone
Copy link
Author

xonstone commented Aug 8, 2019

Yeah, that's what I did but I would or document that this is needed or let singleflight also handle panics hence this issue.

@dmitshur dmitshur changed the title x/sync: panic in singleflight Do fn results in deadlock x/sync/singleflight: panic in Do fn results in deadlock Aug 12, 2019
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2019
@tv42
Copy link

tv42 commented Aug 13, 2019

singleflight might have chosen to not defer recover because of defer performance has historically been pretty noticable and groupcache (where singleflight originates) definitely strived to be fast.

@xonstone
Copy link
Author

At first sight the implementation in groupcache seems to have the same problem

@ayanamist
Copy link

Since go1.14 add inlined defer support, will this be fixed to avoid dead lock?

@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2020

/cc @rsc @ianlancetaylor @dvyukov @aclements per owners.

@dmitshur dmitshur mentioned this issue Sep 1, 2020
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Sep 4, 2020

There was another report with detail that might be relevant in issue #41133. /cc @kirk91

@gopherbot
Copy link

Change https://golang.org/cl/251677 mentions this issue: singleflight: fix hangs after first Do panic

@bcmills
Copy link
Contributor

bcmills commented Sep 30, 2020

Fixed by CL 251677.

@bcmills bcmills closed this as completed Sep 30, 2020
@gopherbot
Copy link

Change https://golang.org/cl/260717 mentions this issue: singleflight: skip tests involving exec on js/wasm

gopherbot pushed a commit to golang/sync that referenced this issue Oct 8, 2020
singleflight: skip tests involving exec on js/wasm
b3e1573 
The js port does not yet support os/exec.

Updates golang/go#37100
Updates golang/go#33519

Change-Id: I9608b7febfdc274dc1b9f34a92d00ef7bea4e13c
Reviewed-on: https://go-review.googlesource.com/c/sync/+/260717
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
sherifabdlnaby pushed a commit to sherifabdlnaby/sync that referenced this issue Feb 24, 2021
@sherifabdlnaby
singleflight: skip tests involving exec on js/wasm
4cf3596 
The js port does not yet support os/exec.

Updates golang/go#37100
Updates golang/go#33519

Change-Id: I9608b7febfdc274dc1b9f34a92d00ef7bea4e13c
Reviewed-on: https://go-review.googlesource.com/c/sync/+/260717
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@mohuishou mohuishou mentioned this issue Jul 7, 2021
Closed
@golang golang locked and limited conversation to collaborators Oct 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
8 participants
@beoran @ayanamist @tv42 @av86743 @dmitshur @bcmills @gopherbot @xonstone