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.

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

runtime: deadlock detector false positive on Windows #56989

Closed
9072997 opened this issue Nov 29, 2022 · 7 comments
Closed

runtime: deadlock detector false positive on Windows #56989

9072997 opened this issue Nov 29, 2022 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@9072997
Copy link

9072997 commented Nov 29, 2022

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

go version go1.19.3 windows/amd64

Does this issue reproduce with the latest release?

I believe 1.19.3 to be the latest release at the moment, so yes.

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

go env Output
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\jpenn\AppData\Local\go-build
set GOENV=C:\Users\jpenn\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\jpenn\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\jpenn\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.19.3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=L:\Work\scratch\tmp\deadlock-example\go.mod
set GOWORK=
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\jpenn\AppData\Local\Temp\go-build4217524297=/tmp/go-build -gno-record-gcc-switches

What did you do?

deadlock-example.zip

See the attached zip file. There are 4 versions of the same program. Version A triggers the deadlock detector. I wanted to test this program with the deadlock detector disabled but AFAIK there is no explicit way to do that, so I created 3 other versions of the program (B, C, and D from the zip file) which avoid the deadlock detector in various ways.

  • Version A is the original version of the program which causes the false positive deadlock detection
  • Version B does include "C", which as far as I can tell, disables the deadlock detector, even if there is no actual C code. This version of the program works as expected, proving that it is not a true deadlock.
  • Version C of the program "distracts" the deadlock detector by starting a 2nd goroutine that does time.Sleep() in a loop. This version of the program works as expected.
  • Version D of the program "distracts" the deadlock detector by starting a TCP listener on a random port in a 2nd goroutine. This version of the program works as expected.

What did you expect to see?

Watching for events...
got an event
got an event
got an event

Note that you do have to wait for something on your Windows computer to actually generate an event. Manually checking for Windows updates is a fairly easy way to trigger one, though be warned the button to check for updates is de-bounced to about once per minute.

What did you see instead?

Watching for events...
fatal error: all goroutines are asleep - deadlock!

goroutine 1 [select]:
main.main()
        L:/Work/scratch/tmp/deadlock-example/a/version-a.go:19 +0x167
exit status 2
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 29, 2022
@bcmills
Copy link
Member

bcmills commented Nov 30, 2022

(CC @golang/runtime @golang/windows)

I suspect that the deadlock detector is triggering because all of the threads created by the Go runtime are blocked, and it doesn't know about other threads created by system calls (such as, in this case, EvtSubscribe).

I wonder: would it suffice to have calls to syscall.NewCallback and syscall.NewCallbackCDecl disable the race deadlock detector?

(Or are there other Windows library functions that create meaningful background threads, don't require cgo, and also don't require a callback argument?)

@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 30, 2022
@bcmills bcmills added this to the Backlog milestone Nov 30, 2022
@9072997
Copy link
Author

9072997 commented Nov 30, 2022

Another relevant question would be: Are syscall.NewCallback and syscall.NewCallbackCDecl the only valid ways to get a function pointer, and I think the answer is technically no.

  • Some things return pointers to functions outside the Go world (ex: syscall.GetProcAddress). You could have a Go program that would plumb these together then select {}. At this point it would be impossible to re-enter the Go world, but useful work could still be happening in the OS thread.
  • Dynamic compilation: I've never seem this done in Go, but in theory you could write executable code directly into a []byte, convert it to a uintptr with unsafe.Pointer, pass that as a callback, and select {}. Again, it becomes impossible to re-enter the Go world, but useful work could still be happening in the OS thread.

Having syscall.NewCallback disable the race detector sounds like a good idea, but it might also make sense to add a function to runtime to explicitly enable/disable the race deadlock detector in case you are doing something really weird.

@bcmills
Copy link
Member

bcmills commented Nov 30, 2022

it might also make sense to add a function to runtime to explicitly enable/disable the [deadlock] detector in case you are doing something really weird.

I think you can already do that fairly easily today by intentionally leaking a timer:

	_ = time.NewTimer(time.Duration(math.MaxInt64)) // Disable deadlock detection.

(https://go.dev/play/p/IeMmf9aTuLO)

@bcmills
Copy link
Member

bcmills commented Nov 30, 2022

At this point it would be impossible to re-enter the Go world, but useful work could still be happening in the OS thread.

Personally I'm OK with the Go runtime flagging that as a deadlock. 😅

convert it to a uintptr with unsafe.Pointer, pass that as a callback

You'd also have to pin in (#46787) and mark it executable.
But unsafe.Pointer is, well, “unsafe”, so again I'm ok with that breaking the deadlock detector. 🙃

@9072997
Copy link
Author

9072997 commented Dec 1, 2022

I think you can do the dynamic compilation thing without unsafe, though the examples become increasingly pathological. I think it comes down to the fact that syscall is about as unsafe as unsafe.

A slightly more normal example might be loading a DLL, CreateThread-ing into it, then doing select {}. This is another "impossible to re-enter the Go world" example.

If we are adopting the position that it is valid for the deadlock detector to kill the process in the "impossible to re-enter the Go world" situation, should that be part of the language spec? My gut feeling would be: fix the case in the original example where it is possible to call back into Go code, and consider the other situations a "wont-fix" sort of bug.

@mknyszek
Copy link
Contributor

mknyszek commented Dec 7, 2022

In triage, we're thinking that maybe what the runtime should do is disable the deadlock detector on any syscalls that take a callback. However, enumerating all of those seems potentially difficult. Alternatively we just disable the deadlock detector on Windows, which would be a shame (though, the deadlock detector isn't particularly sophisticated).

@mknyszek
Copy link
Contributor

mknyszek commented Dec 7, 2022

@prattmic notes that this is a duplicate of #55015.

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
Status: Done
Development

No branches or pull requests

4 participants