Changing recovery function can cause panic #52
Comments
|
I guess we could fix it once and for all by having a SetPanicHandler method with a mutex, but that seems a bit overkill for such an unlikely use-case. |
|
Agreed. And it is still technically an error case (since some panic has to trigger the callback) so the worst case is that we don't get a useful backtrace from the panic. |
eapache
added a commit
that referenced
this issue
Nov 15, 2014
Properly fixes #52 this time. Almost exactly a year ago (+ 3 days) when Flo added the withRecover function we discussed how if the user set/unset the PanicHandler in time with an actual panic they could cause secondary panics and weirdness. We decided putting a mutex in was overkill and closed the issue. It just occured to me that there is a 1000x better fix, which is to just cache the handler value locally so we know we're calling the same value we do our nil-check on. Duh. I'm not sure why neither of us thought of this at the time...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
@fw42 just thought of this. Set the handler, do something that spawns a go routine, then set the handler to nil. A panic would cause a second panic when trying to call a nil function pointer.
Always defer the function, then do the nil check inside the deferred function seems like the fix? There's still a race there, but dunno how to fix that, and it's really unlikely.
The text was updated successfully, but these errors were encountered: