Preventing panics of RecoverableError casts #2267
Conversation
There was a problem hiding this comment.
Thanks for jumping on a fix, and I'm sorry for introducing this bug.
I've left some comments that hopefully guide you toward the solution that avoids re-introducing a past bug where nil errors because non-nil errors.
I can also just take this over if you like.
Thanks again!
nomad/structs/structs.go
Outdated
| @@ -4054,7 +4054,7 @@ type RecoverableError struct { | |||
|
|
|||
| // NewRecoverableError is used to wrap an error and mark it as recoverable or | |||
| // not. | |||
| func NewRecoverableError(e error, recoverable bool) error { | |||
| func NewRecoverableError(e error, recoverable bool) *RecoverableError { | |||
There was a problem hiding this comment.
So, I changed this return value from *RecoverableError to error intentionally in 0.5.3 to avoid turning nil errors into non-nil *RecoverableErrors. Here's an example of that sort of problem: https://play.golang.org/p/Y8z9GdcAhx
So let's keep returning error here to avoid that issue.
There was a problem hiding this comment.
Thanks for the awesome explanation. What a nasty side effect.
nomad/node_endpoint.go
Outdated
| @@ -944,7 +944,7 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest, | |||
| // setErr is a helper for setting the recoverable error on the reply and | |||
| // logging it | |||
| setErr := func(e error, recoverable bool) { | |||
| reply.Error = structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError) | |||
| reply.Error = structs.NewRecoverableError(e, recoverable) | |||
There was a problem hiding this comment.
Instead of omitting the type assertion, let's just check if error is nil and return early.
Otherwise we re-introduce a bug where reply.Error gets set even when the original error e was nil!
There was a problem hiding this comment.
I was going to do a type assertion before I thought I could just change the return type.
Will have shortly.
|
@schmichael see latest |
nomad/node_endpoint.go
Outdated
| @@ -944,7 +944,9 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest, | |||
| // setErr is a helper for setting the recoverable error on the reply and | |||
| // logging it | |||
| setErr := func(e error, recoverable bool) { | |||
| reply.Error = structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError) | |||
| if rerr, ok := structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError); ok { | |||
There was a problem hiding this comment.
This approach will still log an error even when e is nil.
Instead I would just check if e is nil and return early:
setErr := func(...) {
if e == nil { return }
// ...existing code...
}There was a problem hiding this comment.
Oh, right. nil is "castable" to *structs.RecoverableError.
|
Just run Thanks for sticking with this! Handling custom errors in Go is sadly tricky. You'll be the third person to try to get all of this logic right. :) |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #2266