-
Notifications
You must be signed in to change notification settings - Fork 771
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
Replace unsafe usage of recover() in helper functions #4913
Conversation
Now that it's merged, want to see if mgechev/revive#719 can be pulled in easily enough? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 all LGTM. And I am glad that gofmt is permissive about this being one line...
// errPanic MUST be the result from calling recover, which MUST be done in a single level deep | ||
// deferred function. The usual way of calling this is: | ||
// - defer func() { log.CapturePanic(recover(), logger, &err) }() | ||
func CapturePanic(errPanic interface{}, logger Logger, retError *error) { | ||
if errPanic != nil { | ||
err, ok := errPanic.(error) | ||
if !ok { | ||
err = fmt.Errorf("panic object is not error: %#v", errPanic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll lightly vote for a rename, but either's clear enough I think. and there is some benefit in implying something may be panicking error objects and they're handled specially...
eh. your pick.
// errPanic MUST be the result from calling recover, which MUST be done in a single level deep | |
// deferred function. The usual way of calling this is: | |
// - defer func() { log.CapturePanic(recover(), logger, &err) }() | |
func CapturePanic(errPanic interface{}, logger Logger, retError *error) { | |
if errPanic != nil { | |
err, ok := errPanic.(error) | |
if !ok { | |
err = fmt.Errorf("panic object is not error: %#v", errPanic) | |
// "recovered" MUST be the result from calling recover, which MUST be done in a single level deep | |
// deferred function. The usual way of calling this is: | |
// - defer func() { log.CapturePanic(recover(), logger, &err) }() | |
func CapturePanic(recovered interface{}, logger Logger, retError *error) { | |
if recovered != nil { | |
err, ok := recovered.(error) | |
if !ok { | |
err = fmt.Errorf("panic object is not error: %#v", recovered) |
We decided that our usage of helper functions when using defer/recover was too brittle, since one too many functions causes recover() not to do the right thing in Go. Switching to this idiom, where the defer caller also calls recover() ensures that we will get the right result from recover(), but also lets us continue to use the helper functions to deduplicate the code that runs after recover().
We decided that our usage of helper functions when using defer/recover was too brittle, since one
too many functions causes recover() not to do the right thing in Go. Switching to this idiom, where
the defer caller also calls recover() ensures that we will get the right result from recover(), but
also lets us continue to use the helper functions to deduplicate the code that runs after recover().