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: wait for active panic before exit on Windows #20018

Closed
sydnash opened this issue Apr 18, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@sydnash
Copy link

commented Apr 18, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8

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

windows/amd64
$ go env
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=D:\gowork
set GORACE=
set GOROOT=D:\Go
set GOTOOLDIR=D:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

p.go

package main
import (
	"sync"
)

func run(wg *sync.WaitGroup) {
	defer wg.Done()
	panic("boom")
}

func main() {
	var wg sync.WaitGroup
	wg.Add(1)
	go run(&wg)
	wg.Wait()
}

What did you expect to see?

I want to see the output information as blow ever time i run it.
panic: boom

goroutine 17 [running]:
main.run(0xc042040000)
D:/gowork/src/github.com/sydnash/practice/select/p.go:9 +0x95
created by main.main
D:/gowork/src/github.com/sydnash/practice/select/p.go:15 +0x72
exit status 2

What did you see instead?

I found it some times exit with status 0 and without panic information output.

@mvdan mvdan changed the title issue 3934 on win7 and64 runtime: issue 3934 on win7 amd64 Apr 18, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

I can't reproduce it on linux/amd64 on either tip nor 1.8.1, so likely related to Windows.

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 18, 2017

@mvdan Yes, it's ok on Linux. I only test it on Windows and linux, and linux is ok.

@ianlancetaylor ianlancetaylor changed the title runtime: issue 3934 on win7 amd64 runtime: wait for active panic before exit on Windows Apr 18, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.9Maybe milestone Apr 18, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

No idea why this fails on Windows. The fix for #3934 (https://golang.org/cl/7322083) was not OS-specific. Somebody with a Windows system will have to debug what is happening.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 19, 2017

the runtime.main function not enter gopark branch at all if there is no panic information output.

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 19, 2017

if panicking != 0 {
		println("main.main park")
		gopark(nil, nil, "panicwait", traceEvGoStop, 1)
	}

	print("main.main exit with panicking : ", panicking, "\n")

i modify runtime.main like this, and when no panic information,
println("main.main park") is not output
and print("main.main exit with panicking : ", panicking, "\n") print 0 or 1 randomly.
@ianlancetaylor so i think this issue is not the same as 3934.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

Thanks for looking into it. I think I see the problem, but I don't understand why it only appears on Windows. The problem is that calling panic runs deferred functions before setting the runtime package variable panicking. It has to be that way, because we only want to set panicking if we are going to exit, but if a deferred function recovers the panic then we will not exit. In this case the deferred function is wg.Done, and calling it permits wg.Wait to complete, and thus permits main to exit.

I can't quite decide how important it is to fix this problem.

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 19, 2017

yes, i doubt it also for that only appears on Windows, today i check it on linux again and it's ok.
dose we can add a userpanicing for user's panic call, and inc it immediately after enter gopanic, and dec it after startpanic() or before recover.
and check userpanicing also before check panicing, and call a goparm(). some diff like this:

--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -412,6 +412,9 @@ func printpanics(p *_panic) {

 // The implementation of the predeclared function panic.
 func gopanic(e interface{}) {
+       lock(&userpaniclk)
+       atomic.Xadd(&userpanicing, 1)
+       unlock(&userpaniclk)
        gp := getg()
        if gp.m.curg != gp {
                print("panic: ")
@@ -516,6 +519,9 @@ func gopanic(e interface{}) {
                        // Pass information about recovering frame to recovery.
                        gp.sigcode0 = uintptr(sp)
                        gp.sigcode1 = pc
+                       lock(&userpaniclk)
+                       atomic.Xadd(&userpanicing, -1)
+                       unlock(&userpaniclk)
                        mcall(recovery)
                        throw("recovery failed") // mcall should not return
                }
@@ -526,7 +532,13 @@ func gopanic(e interface{}) {
        // the world, we call preprintpanics to invoke all necessary Error
        // and String methods to prepare the panic strings before startpanic.
        preprintpanics(gp._panic)
+       println("gopanic: start panic")
        startpanic()
+
+       lock(&userpaniclk)
+       atomic.Xadd(&userpanicing, -1)
+       unlock(&userpaniclk)
+
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -191,10 +191,22 @@ func main() {
        // another goroutine at the same time as main returns,
        // let the other goroutine finish printing the panic trace.
        // Once it does, it will exit. See issue 3934.
+       lock(&userpaniclk)
+       println("check userpanicing")
+       if userpanicing != 0 {
+               unlock(&userpaniclk)
+               gopark(nil, nil, "panicwait", traceEvGoStop, 1)
+       } else {
+               unlock(&userpaniclk)
+       }
+       println("main.main is exit now, check paniking start, ", panicking)
        if panicking != 0 {
+               println("main.main park")
                gopark(nil, nil, "panicwait", traceEvGoStop, 1)
        }

+       print("main.main exit with panicking : ", panicking, "\n")
+
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -750,6 +750,9 @@ var (

        goarm                uint8 // set by cmd/link on arm systems
        framepointer_enabled bool  // set by cmd/link
+
+       userpaniclk  mutex
+       userpanicing uint32

I don't know dose it ok on recover.
but i test this on the origin error code mentioned before, it's ok.

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 19, 2017

and we check dose main.main is exit when recover, if it is, change runtime.mian to run again.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

I think I see the problem, but I don't understand why it only appears on Windows.

I tried to debug this a little. I see 2 threads racing to exit process:

  1. runtime.dopanic_m is ending with runtime.exit(2)
  2. runtime.main is ending with runtime.exit(0)

@ianlancetaylor if you have some things for me to try, let me know.

Alex

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

@alexbrainman Can you see if https://golang.org/cl/41052 fixes the problem on Windows?

I was able to recreate the problem on GNU/Linux by adding a runtime.Gosched in the deferred function.

This might be too much mechanism for a racy program. We'll see what other people think.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

Can you see if https://golang.org/cl/41052 fixes the problem on Windows?

Yes, that fixes this issue here. Thank you.

This might be too much mechanism for a racy program.

I feel the same. Let Austin decide if it is worth it.

Alex

@sydnash

This comment has been minimized.

Copy link
Author

commented Apr 20, 2017

@alexbrainman @ianlancetaylor
if we have three threads, A is for main function, B is for panicking goroutinue, C is for continue main.
C continued A while B is panicking and enter recover not yet.
then A get a chance to run and main will exit 0 before B's panic information can output.

but i think this will hard to appear.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@sydnash Yes. The program is racy.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 21, 2017

CL https://golang.org/cl/41052 mentions this issue.

@gopherbot gopherbot closed this in 2d86f49 Jun 5, 2017

@golang golang locked and limited conversation to collaborators Jun 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.