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
spec: clarify when calling recover stops a panic #34530
Comments
If this is the intended behavior, then the descriptions in the spec doesn't cover this case. |
Tentatively milestoning for 1.14 on the theory that it may be addressed alongside #34481, but feel free to re-milestone if that is not realistic. |
@go101, personally I would expect this program to crash with panic Since both of the But certainly a clarification either way would be helpful. |
By the explanations for recover mechanism in spec, the following program has no fundamental differences from the above one. But the following one exits without crashing. package main
import "fmt"
func demo() {
defer func() {
// recover panic 1
defer fmt.Println(" (done).")
defer recover()
defer fmt.Print("To recover panic 1 ...")
defer func() {
// recover panic 2
fmt.Println("panic", recover(), "is recovered")
}()
defer fmt.Println("now, two active panics coexist")
panic(2)
}()
panic(1)
}
func main() {
demo()
} My understanding by reading spec is a |
Yes, the Go language spec is very vague and imprecise on panic/recover and I am hoping to fix it: https://go-review.googlesource.com/c/go/+/189377 but there's not quite consensus yet. I don't think there is any disagreement on the behavior of your example (unlike the abort behavior mentioned in #29226 ). A recover only recovers a panic if it is called directly in a defer function that is directly invoked as part of the panicking process of that panic. A recover does not apply and returns nil if it is not called directly in a defer function or if it is called from a defer that was not directly invoked by the panic sequence of the panic (i.e. is nested inside some other defer or function called by the defer). Your second example seems to be a special little bug/quirk of doing 'defer recover()'. The detection mechanism in the implementation considers that recover is called directly in the containing function, so that recover does apply. Note that recover does not happen if you do
I think the first thing in both these bugs are to get agreement on the spec - both specifying the current behavior that people depend on and/or would generally agree on, and also the behavior (like the abort behavior in #29226 ) that has been there for a while, but has never been specified and people might like to change. |
Yes, this is a little quirk. There is not a way to verify whether or not the recovered value is really |
The difference between the two programs has to do with the behavior of So, in the first program, the In the second program, the |
@ianlancetaylor Your explanation makes me more believe that each goroutine maintains a panic stack. Only the top panic in the stack may be recovered. And to be recoverable, the top panic must not be created in So my current understanding is like this:
Is the understanding right? [edit]: there may be some blank slots in the panic stack, or each panic will be associated with a depth property. The depth of a panic can decrease and will never be larger than the current execution frame depth. |
@ianlancetaylor |
Sorry, I don't really understand your frame depth discussion. I encourage you to comment on the CL where you think it needs adjustment. |
I think this is a really interesting test case. Thanks for reporting it, @go101. On my phone, so I haven't looked at the Go spec wording or @danscales's suggestion recently, so I'm not sure the right behaviour. From memory though, I can see arguments either way. |
Thanks for all the examples and discussion. I missed that there was a With respect to talking about (and spec'ing out) the behavior of panic/recover/recursive panics, it will be best not to use |
Are we okay breaking Go 1 compat here? There's at least some code that uses |
There are two changes we can make without breaking compatibility:
|
That use of |
I believe so. |
Personally, I think the cost of either vet or ban [edit 2]: the validity of the |
On the other hand, for consistency, it is not a bad idea to ban discarding returns of built-in functions. Currently, only the returns of |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What did you do?
What did you expect to see?
Not crash.
What did you see instead?
Crashes.
The text was updated successfully, but these errors were encountered: