Skip to content

Commit

Permalink
Include valid spec and headers when calling recover handler for strea…
Browse files Browse the repository at this point in the history
…ming RPCs (#817)

For some reason, this was passing an empty spec and nil headers for
streaming RPCs, though the Go docs don't suggest anything of the sort.

Also simplified the panic recovery to use more intuitive idioms, which
now suffice since this module requires Go 1.21 (which no longer allows
panicking with a nil value).
  • Loading branch information
jhump authored Feb 5, 2025
1 parent 48588fe commit 720d197
Showing 1 changed file with 3 additions and 19 deletions.
22 changes: 3 additions & 19 deletions recover.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,6 @@ import (
// recoverHandlerInterceptor lets handlers trap panics, perform side effects
// (like emitting logs or metrics), and present a friendlier error message to
// clients.
//
// This interceptor uses a somewhat unusual strategy to recover from panics.
// The standard recovery idiom:
//
// if r := recover(); r != nil { ... }
//
// isn't robust in the face of user error, because it doesn't handle
// panic(nil). This occasionally happens by mistake, and it's a beast to debug
// without a more robust idiom. See https://github.com/golang/go/issues/25448
// for details.
type recoverHandlerInterceptor struct {
Interceptor

Expand All @@ -43,10 +33,8 @@ func (i *recoverHandlerInterceptor) WrapUnary(next UnaryFunc) UnaryFunc {
if req.Spec().IsClient {
return next(ctx, req)
}
panicked := true
defer func() {
if panicked {
r := recover()
if r := recover(); r != nil {
// net/http checks for ErrAbortHandler with ==, so we should too.
if r == http.ErrAbortHandler { //nolint:errorlint,goerr113
panic(r) //nolint:forbidigo
Expand All @@ -55,26 +43,22 @@ func (i *recoverHandlerInterceptor) WrapUnary(next UnaryFunc) UnaryFunc {
}
}()
res, err := next(ctx, req)
panicked = false
return res, err
}
}

func (i *recoverHandlerInterceptor) WrapStreamingHandler(next StreamingHandlerFunc) StreamingHandlerFunc {
return func(ctx context.Context, conn StreamingHandlerConn) (retErr error) {
panicked := true
defer func() {
if panicked {
r := recover()
if r := recover(); r != nil {
// net/http checks for ErrAbortHandler with ==, so we should too.
if r == http.ErrAbortHandler { //nolint:errorlint,goerr113
panic(r) //nolint:forbidigo
}
retErr = i.handle(ctx, Spec{}, nil, r)
retErr = i.handle(ctx, conn.Spec(), conn.RequestHeader(), r)
}
}()
err := next(ctx, conn)
panicked = false
return err
}
}

0 comments on commit 720d197

Please sign in to comment.