Skip to content

fix: return codes.Internal on recovered panic instead of nil error #46

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.

Sign up for GitHub

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

Open
janwytze wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: return codes.Internal on recovered panic instead of nil error #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions server_interceptors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,15 @@ import (
"google.golang.org/grpc"
)

func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options) {
// recoverWithSentry recovers from a panic in the handler, reports it to Sentry
// and, unless Repanic is enabled, translates it into a codes.Internal error via
// outErr (and marks the transaction as failed).
//
// Without this translation the interceptor would fall through to its zero-value
// return — a nil error — making gRPC treat the RPC as successful: a streaming
// handler reports OK with a missing/partial response, and a unary handler
// returns a nil response that fails to marshal with an opaque Internal error.
func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options, tx *sentry.Span, outErr *error) {
if err := recover(); err != nil {
eventID := hub.RecoverWithContext(ctx, err)
if eventID != nil && o.WaitForDelivery {
Expand All @@ -26,6 +34,13 @@ func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options) {
if o.Repanic {
panic(err)
}

if outErr != nil {
*outErr = status.Errorf(codes.Internal, "%v", err)
}
if tx != nil {
tx.Status = sentry.SpanStatusInternalError
}
}
}

Expand All @@ -34,7 +49,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor {
return func(ctx context.Context,
req interface{},
info *grpc.UnaryServerInfo,
handler grpc.UnaryHandler) (interface{}, error) {
handler grpc.UnaryHandler) (resp interface{}, err error) {

hub := sentry.GetHubFromContext(ctx)
if hub == nil {
Expand Down Expand Up @@ -67,9 +82,9 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor {
// TODO: Perhaps makes sense to use SetRequestBody instead?
hub.Scope().SetExtra("requestBody", req)
}
defer recoverWithSentry(hub, ctx, o)
defer recoverWithSentry(hub, ctx, o, tx, &err)

resp, err := handler(ctx, req)
resp, err = handler(ctx, req)
if err != nil && o.ReportOn(err) {
tags := grpc_tags.Extract(ctx)
for k, v := range tags.Values() {
Expand All @@ -92,7 +107,7 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor {
return func(srv interface{},
ss grpc.ServerStream,
info *grpc.StreamServerInfo,
handler grpc.StreamHandler) error {
handler grpc.StreamHandler) (err error) {

ctx := ss.Context()
hub := sentry.GetHubFromContext(ctx)
Expand Down Expand Up @@ -125,9 +140,9 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor {
stream := grpc_middleware.WrapServerStream(ss)
stream.WrappedContext = ctx

defer recoverWithSentry(hub, ctx, o)
defer recoverWithSentry(hub, ctx, o, tx, &err)

err := handler(srv, stream)
err = handler(srv, stream)
if err != nil && o.ReportOn(err) {
tags := grpc_tags.Extract(ctx)
for k, v := range tags.Values() {
Expand Down
112 changes: 106 additions & 6 deletions server_interceptors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
)

// mockUnaryHandler is a mock handler for testing unary interceptors
Expand Down Expand Up @@ -44,12 +45,12 @@ type mockServerStream struct {
ctx context.Context
}

func (m *mockServerStream) SetHeader(metadata.MD) error { return nil }
func (m *mockServerStream) SendHeader(metadata.MD) error { return nil }
func (m *mockServerStream) SetTrailer(metadata.MD) {}
func (m *mockServerStream) Context() context.Context { return m.ctx }
func (m *mockServerStream) SendMsg(interface{}) error { return nil }
func (m *mockServerStream) RecvMsg(interface{}) error { return nil }
func (m *mockServerStream) SetHeader(metadata.MD) error { return nil }
func (m *mockServerStream) SendHeader(metadata.MD) error { return nil }
func (m *mockServerStream) SetTrailer(metadata.MD) {}
func (m *mockServerStream) Context() context.Context { return m.ctx }
func (m *mockServerStream) SendMsg(interface{}) error { return nil }
func (m *mockServerStream) RecvMsg(interface{}) error { return nil }

func TestUnaryServerInterceptor_Configuration(t *testing.T) {
// Test that the interceptor can be created with different options
Expand Down Expand Up @@ -162,6 +163,105 @@ func TestStreamServerInterceptor_Configuration(t *testing.T) {
}
}

// tracedIncomingContext returns a server-side context carrying a valid
// sentry-trace header, mirroring a real request whose caller propagates the
// trace. Without a trace header, StartTransaction is invoked with a nil
// SpanOption from ContinueFromGrpcMetadata, which is an unrelated concern.
func tracedIncomingContext() context.Context {
md := metadata.New(map[string]string{
sentry.SentryTraceHeader: "1234567890abcdef1234567890abcdef-1234567890abcdef-1",
})
return metadata.NewIncomingContext(context.Background(), md)
}

func TestUnaryServerInterceptor_RecoversPanicAsInternal(t *testing.T) {
// With the default options (Repanic disabled), a panicking handler must be
// recovered and returned as a codes.Internal error rather than a nil error.
interceptor := UnaryServerInterceptor()
handler := &mockUnaryHandler{panic: true}
info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"}

resp, err := interceptor(tracedIncomingContext(), "request", info, handler.handle)

if resp != nil {
t.Errorf("Expected nil response on panic, got %v", resp)
}
if got := status.Code(err); got != codes.Internal {
t.Fatalf("Expected codes.Internal, got %v (err=%v)", got, err)
}
}

func TestUnaryServerInterceptor_RepanicsWhenEnabled(t *testing.T) {
// With Repanic enabled the panic must propagate to the caller unchanged.
interceptor := UnaryServerInterceptor(WithRepanicOption(true))
handler := &mockUnaryHandler{panic: true}
info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"}

defer func() {
if r := recover(); r == nil {
t.Error("Expected interceptor to re-panic, but it did not")
}
}()
_, _ = interceptor(tracedIncomingContext(), "request", info, handler.handle)
}

func TestUnaryServerInterceptor_PassesThroughError(t *testing.T) {
// A normal handler error must be returned unchanged.
interceptor := UnaryServerInterceptor()
handler := &mockUnaryHandler{err: status.Error(codes.NotFound, "missing")}
info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"}

_, err := interceptor(tracedIncomingContext(), "request", info, handler.handle)

if got := status.Code(err); got != codes.NotFound {
t.Fatalf("Expected codes.NotFound to pass through, got %v (err=%v)", got, err)
}
}

func TestStreamServerInterceptor_RecoversPanicAsInternal(t *testing.T) {
// With the default options (Repanic disabled), a panicking handler must be
// recovered and returned as a codes.Internal error rather than a nil error.
interceptor := StreamServerInterceptor()
handler := &mockStreamHandler{panic: true}
info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"}
stream := &mockServerStream{ctx: tracedIncomingContext()}

err := interceptor(nil, stream, info, handler.handle)

if got := status.Code(err); got != codes.Internal {
t.Fatalf("Expected codes.Internal, got %v (err=%v)", got, err)
}
}

func TestStreamServerInterceptor_RepanicsWhenEnabled(t *testing.T) {
// With Repanic enabled the panic must propagate to the caller unchanged.
interceptor := StreamServerInterceptor(WithRepanicOption(true))
handler := &mockStreamHandler{panic: true}
info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"}
stream := &mockServerStream{ctx: tracedIncomingContext()}

defer func() {
if r := recover(); r == nil {
t.Error("Expected interceptor to re-panic, but it did not")
}
}()
_ = interceptor(nil, stream, info, handler.handle)
}

func TestStreamServerInterceptor_PassesThroughError(t *testing.T) {
// A normal handler error must be returned unchanged.
interceptor := StreamServerInterceptor()
handler := &mockStreamHandler{err: status.Error(codes.NotFound, "missing")}
info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"}
stream := &mockServerStream{ctx: tracedIncomingContext()}

err := interceptor(nil, stream, info, handler.handle)

if got := status.Code(err); got != codes.NotFound {
t.Fatalf("Expected codes.NotFound to pass through, got %v (err=%v)", got, err)
}
}

func TestContinueFromGrpcMetadata(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading