Skip to content

Commit 720d197

Browse files
authored
Include valid spec and headers when calling recover handler for streaming 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).
1 parent 48588fe commit 720d197

File tree

1 file changed

+3
-19
lines changed

1 file changed

+3
-19
lines changed

recover.go

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,6 @@ import (
2222
// recoverHandlerInterceptor lets handlers trap panics, perform side effects
2323
// (like emitting logs or metrics), and present a friendlier error message to
2424
// clients.
25-
//
26-
// This interceptor uses a somewhat unusual strategy to recover from panics.
27-
// The standard recovery idiom:
28-
//
29-
// if r := recover(); r != nil { ... }
30-
//
31-
// isn't robust in the face of user error, because it doesn't handle
32-
// panic(nil). This occasionally happens by mistake, and it's a beast to debug
33-
// without a more robust idiom. See https://github.com/golang/go/issues/25448
34-
// for details.
3525
type recoverHandlerInterceptor struct {
3626
Interceptor
3727

@@ -43,10 +33,8 @@ func (i *recoverHandlerInterceptor) WrapUnary(next UnaryFunc) UnaryFunc {
4333
if req.Spec().IsClient {
4434
return next(ctx, req)
4535
}
46-
panicked := true
4736
defer func() {
48-
if panicked {
49-
r := recover()
37+
if r := recover(); r != nil {
5038
// net/http checks for ErrAbortHandler with ==, so we should too.
5139
if r == http.ErrAbortHandler { //nolint:errorlint,goerr113
5240
panic(r) //nolint:forbidigo
@@ -55,26 +43,22 @@ func (i *recoverHandlerInterceptor) WrapUnary(next UnaryFunc) UnaryFunc {
5543
}
5644
}()
5745
res, err := next(ctx, req)
58-
panicked = false
5946
return res, err
6047
}
6148
}
6249

6350
func (i *recoverHandlerInterceptor) WrapStreamingHandler(next StreamingHandlerFunc) StreamingHandlerFunc {
6451
return func(ctx context.Context, conn StreamingHandlerConn) (retErr error) {
65-
panicked := true
6652
defer func() {
67-
if panicked {
68-
r := recover()
53+
if r := recover(); r != nil {
6954
// net/http checks for ErrAbortHandler with ==, so we should too.
7055
if r == http.ErrAbortHandler { //nolint:errorlint,goerr113
7156
panic(r) //nolint:forbidigo
7257
}
73-
retErr = i.handle(ctx, Spec{}, nil, r)
58+
retErr = i.handle(ctx, conn.Spec(), conn.RequestHeader(), r)
7459
}
7560
}()
7661
err := next(ctx, conn)
77-
panicked = false
7862
return err
7963
}
8064
}

0 commit comments

Comments
 (0)