grpc: Interceptor to catch panics and convert them to Internal errors.

grpc/grpcmon/server.go

 // Copyright 2016 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package grpcmon
 
 import (
         "time"
 
         "golang.org/x/net/context"
         "google.golang.org/grpc"
+        "google.golang.org/grpc/codes"
 
         "github.com/luci/luci-go/common/clock"
         "github.com/luci/luci-go/common/tsmon/distribution"
         "github.com/luci/luci-go/common/tsmon/field"
         "github.com/luci/luci-go/common/tsmon/metric"
         "github.com/luci/luci-go/common/tsmon/types"
 )
 
 var (
         grpcServerCount = metric.NewCounter(
@@ skipping 15 matching lines @@
 // NewUnaryServerInterceptor returns an interceptor that gathers RPC handler
 // metrics and sends them to tsmon.
 //
 // It can be optionally chained with other interceptor. The reported metrics
 // include time spent in this other interceptor too.
 //
 // It assumes the RPC context has tsmon initialized already.
 func NewUnaryServerInterceptor(next grpc.UnaryServerInterceptor) grpc.UnaryServerInterceptor {
         return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
                 started := clock.Now(ctx)
+                panicing := true
                 defer func() {
-                        reportServerRPCMetrics(ctx, info.FullMethod, err, clock.Now(ctx).Sub(started))
+                        // We don't want to recover anything, but we want to log Internal error
+                        // in case of a panic. We pray here reportServerRPCMetrics is very
+                        // lightweight and it doesn't panic itself.
+                        code := codes.OK
+                        switch {
+                        case err != nil:
+                                code = grpc.Code(err)
+                        case panicing:
+                                code = codes.Internal

[Vadim Sh., 2017/06/20 04:30:12]

I think it used to log it as codes.OK because it saw err is nil.

While using dedicated panic catching interceptor is better, I think correctly
handling panics here is also important.

+                        }
+                        reportServerRPCMetrics(ctx, info.FullMethod, code, clock.Now(ctx).Sub(started))
                 }()
                 if next != nil {
-                        return next(ctx, req, info, handler)
+                        resp, err = next(ctx, req, info, handler)
+                } else {
+                        resp, err = handler(ctx, req)
                 }
-                return handler(ctx, req)
+                panicing = false // normal exit, no panic happened, disarms defer
+                return
         }
 }
 
 // reportServerRPCMetrics sends metrics after RPC handler has finished.
-func reportServerRPCMetrics(ctx context.Context, method string, err error, dur time.Duration) {
-        code := 0
-        if err != nil {
-                code = int(grpc.Code(err))
-        }
-        grpcServerCount.Add(ctx, 1, method, code)
-        grpcServerDuration.Add(ctx, float64(dur.Nanoseconds()/1e6), method, code)
+func reportServerRPCMetrics(ctx context.Context, method string, code codes.Code, dur time.Duration) {
+        grpcServerCount.Add(ctx, 1, method, int(code))
+        grpcServerDuration.Add(ctx, float64(dur.Nanoseconds()/1e6), method, int(code))
 }