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: stacktrace for deferred code which panics is too subtle for me #45794

Open
seebs opened this issue Apr 27, 2021 · 5 comments
Open

runtime: stacktrace for deferred code which panics is too subtle for me #45794

seebs opened this issue Apr 27, 2021 · 5 comments

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Apr 27, 2021

https://play.golang.org/p/dS4pD8LBsfH

Reproduced with 1.16.3.

What did you expect to see?

Any way to guess what was deferred, or ideally, where it was deferred.

If you substitute a function literal, like defer func() { foo.mu.Unlock() }(), you get to see the name as [function].func1, which helps a lot, but if you just have a direct call to something in runtime, it's pretty hard to read.

But also, I'd like to see the original "write to nil map" panic and not have it get swallowed by the second panic from the deferred code.

What did you see instead?

A stack trace which looks like something in runtime double-unlocked, even though actually it's my code that has the bug, and no reference to the line of code where the defer was.

It's conceivable that a clearer unwinding of this could show some of that information, although it's a bit tricky to express clearly. This might actually be two distinct requests for changes to the stack trace, one for "the defer should be annotated with its source line", and one for "it'd be nice not to lose the original panic". I'm not sure.

@randall77
Copy link
Contributor

@randall77 randall77 commented Apr 27, 2021

A lot of what you ask for is there, albeit somewhat hidden.

Halfway down your stack trace, there is a panic. That's the nil map panic. The subsequent entries are the stack from there until the double-unlock throw.

I'm surprised there's nothing at line 16. I thought we made defers look like they were called at the closing bracket of the function that deferred them. Maybe that's only defers called from the non-panic path, though.

I feel like there might be something to fix here, but I'm not sure what concrete steps that would involve.

@seebs
Copy link
Contributor Author

@seebs seebs commented Apr 27, 2021

I'm not either, I just got really confused. (The original case was a lot larger than this, but involved a deferred unlock getting hit by a panic from a tiny region which was dropping-and-reclaiming the lock to avoid a deadlock...)

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 27, 2021

Maybe that's only defers called from the non-panic path, though.

Yes. For panicking case the return statement (or the implicit return at the end of the function) is not executed, and the deferred the function is not triggered from there.

This example is also interesting in that it is a runtime fatal error, instead of an ordinary user panic. For an ordinary panic, many intermediate runtime frames are not shown: https://play.golang.org/p/nlGlx9cSQ5y

@cherrymui cherrymui added this to the Unplanned milestone Apr 27, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 27, 2021

Maybe double-unlock of a user lock (not a runtime lock) can be treated as user panic instead of fatal error? Or at least print stack traces in the user-panic way?

@cherrymui cherrymui changed the title stacktrace for deferred code which panics is too subtle for me runtime: stacktrace for deferred code which panics is too subtle for me Apr 27, 2021
@seebs
Copy link
Contributor Author

@seebs seebs commented Apr 27, 2021

It seems like the panic from unlock behaves differently from other panics, yeah. Swapping in other kinds of panic (including nil map assignments) produces much clearer stack traces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants