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: a runtime.Goexit call will cancel a panic, which seems unexpected/incorrect #35378

Open
danscales opened this issue Nov 5, 2019 · 7 comments
Milestone

Comments

@danscales
Copy link

@danscales danscales commented Nov 5, 2019

In go 1.13, but probably any version of go, a runtime.Goexit call can cancel a panic (example from @go101):

package main

import "runtime"

func main() {
	c := make(chan struct{})
	go func() {
		defer close(c)
		// The Goexit signal shadows the
		// "bye" panic, but it should not.
		defer runtime.Goexit()
		panic("bye")
	}()
	<-c
}

When you run this program, instead of getting a panic that terminates the program, you get a normal termination with no error. And generally, when a goroutine is panicking, a call to runtime.Goexit will just end the goroutine, but will cancel the panic, so the whole program may continue running.

This is not a big deal, since it has been like this for a long time and people are unlikely to run into it, but seems unexpected/incorrect.

One solution would be just to say that runtime.Goexit() is a no-op if we are in the middle of a panicking sequence. The slight downside to such a solution is that turning runtime.Goexit() to a no-op might be surprising and violate some programmer assumptions (especially if the runtime.Goexit call is buried deep within some other function/module), maybe leading to a second panic.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 5, 2019

One solution would be just to say that runtime.Goexit() is a no-op if we are in the middle of a panicking sequence.

I don't think that would be quite right. It would suppress the Goexit (and allow the goroutine to continue running) if the panic is eventually recovered.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2019

It does seem that a call to Goexit should not stop us from crashing due to a panic. If you believe that then I think the only question is: what should happen if we panic, then do a deferred call to Goexit, then recover the panic. I don't see any obvious answer. It doesn't seem obvious that Goexit should be ignored, and it doesn't seem obvious that Goexit should come back to life and cause the goroutine to exit. So I'm inclined to think we can just pick whichever behavior we like.

Anybody see a clear argument for a particular choice?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 5, 2019

Conceptually, I think either:

  • Goexit during a panic should suppress the panic, causing the goroutine to exit without error unless it panics again while executing deferred calls,

or

  • any Goexit should remain permanently below the panic stack, so that if all panics are recovered the goroutine resumes the Goexit, but if any panic propagates out of the topmost call of the goroutine then the program crashes.

The former seems to be what happens today, so absent a strong argument to the contrary perhaps we should keep it.

(On the other hand, I would find the latter somewhat more intuitive, and it would be much less likely to mask an unintended panic during testing.)

@danscales

This comment has been minimized.

Copy link
Author

@danscales danscales commented Nov 5, 2019

any Goexit should remain permanently below the panic stack, so that if all panics are recovered the goroutine resumes the Goexit, but if any panic propagates out of the topmost call of the goroutine then the program crashes.

This option would probably be quite tricky to implement, but first there are a bunch of details that would have to be specified:

  • When the runtime.Goexit is called (after a panic has happened), do we continuing executing normal code after the Goexit, or do we immediately end the defer that the Goexit was running in and continue to the next defer in the panicking sequence?

  • If all panics are recovered, and we return to processing the Goexit, what does the stack look like at that point in runtime.Callers(). Does the stack have to look exactly like what it was when the Goexit was first called?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

I don't really see why a deferred call to Goexit should linger after the call to panic. To me it seems equally plausible for Goexit to start running and say "ah, we are panicking, so just return rather than exit the goroutine."

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 6, 2019

When the runtime.Goexit is called (after a panic has happened), do we continuing executing normal code after the Goexit, or do we immediately end the defer that the Goexit was running in and continue to the next defer in the panicking sequence?

We should immediately end the defer that called Goexit, the same as we would do if the deferred call invoked panic.

If all panics are recovered, and we return to processing the Goexit, what does the stack look like at that point in runtime.Callers(). Does the stack have to look exactly like what it was when the Goexit was first called?

That I do not know. To my knowledge we haven't specified what the stack looks like during a Goexit, but in general I would expect the top frame to have an accurate Goexit call frame. If there are chunks missing (due to a recover sequence recovering them), I would expect the missing chunk(s) to be indicated somehow.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 6, 2019

I don't really see why a deferred call to Goexit should linger after the call to panic.

The doc for runtime.Goexit says, front and center, “Goexit terminates the goroutine that calls it.” Given that, I don't think it can reasonably fail to terminate the goroutine, even if it looks like the goroutine might end up terminating anyway.

And if runtime.Goexit could fail to exit the goroutine, I would expect it to have a different name that indicates the actual behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.