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

Closed
danscales opened this issue Nov 5, 2019 · 15 comments
Closed
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@danscales
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 6, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@rsc rsc changed the title runtime: a runtime.Goexit call will cancel a panic, which seems unexpected/incorrect runtime: Goexit call will cancel a panic, which seems unexpected/incorrect Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

@bcmills and I spent a little while thinking and talking through this today.

To me, if a deferred handler calls os.Exit in a panic, clearly the process exits and basically cancels the panic (there is no panic dump etc). So if a deferred handler calls runtime.Goexit in a panic, it seems OK to me that the goroutine exits "cleanly" rather than preserving the panic. That is, Goexit cancels a panic.

On the other hand it's clear that panic/recover should not cancel Goexit, which was #35377 and is fixed.

Based on this logic and trying some test cases, @bcmills and I believe this issue is working as intended and can be closed.

@bcmills
Copy link
Contributor

bcmills commented Jul 20, 2022

I double-checked the behavior in a couple of Playground programs.

https://go.dev/play/p/Adxb5z9XBvO looks reasonable to me: the runtime.Goexit cancels the panic that was in flight, just like how an os.Exit would equivalently cancel a panic from the same goroutine, a subsequent panic can be initiated and recovered within the deferred calls, and recovering the panic doesn't cancel the Goexit.

https://go.dev/play/p/vmSAgH-DZgF looks wrong, but in the opposite direction from the title of this issue. The from before Goexit panic should be cleared by the Goexit, but when the program terminates it somehow resurfaces in the stack dump.

@go101
Copy link

go101 commented Jul 21, 2022

So the following functions are equivalent from behavior view?

func foo() {
	defer runtime.Goexit()
	panic(1)
}

func bar() {
	defer func() {recover()}()
	panic(1)
}

func que() {
	defer func() {
		func() {
			runtime.Goexit()
		}()
	}()
	panic(1)
}

@go101
Copy link

go101 commented Jul 21, 2022

If this is the final decision. I recommend to implement Goexit as harmless panic, to simplify the implementation. If the final uncaught panic is a Goexit, then program will not crash (maybe this is just the current implementation).

@bcmills
Copy link
Contributor

bcmills commented Jul 21, 2022

@go101, foo and que are equivalent, but bar is different.

foo initiates a panic and then terminates the goroutine with a Goexit. If it is called by another function and that function defers a function that calls recover, it should recover nil and continue to unwind the stack.
(https://go.dev/play/p/-zLtNCjDkcv shows that it does so correctly, and https://go.dev/play/p/C0cZQuuiplw shows equivalent behavior for que.)

bar initiates a panic but then recovers it. If it is called by another function, execution of that function should resume after the call (not continue to unwind). That is the difference between a Goexit and a panic.
(https://go.dev/play/p/r7AEQWGFPgr)

@go101
Copy link

go101 commented Jul 21, 2022

Yes, Goexit is unrecoverable painc.

I mean those 3 functions are equivalent if they are used as goroutine start functions.

@rsc rsc closed this as completed Aug 2, 2022
@go101
Copy link

go101 commented Aug 16, 2022

Maybe, the spec also needs to update to be accurate.

@go101
Copy link

go101 commented Sep 2, 2022

I recommend to implement Goexit as harmless panic

I just realized things are not so simple. Goexit sometimes behaves like panic, sometimes not.

The final decision really twists the explanations for panic/recover mechanism to some degree.

@golang golang locked and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants