From 4ad658d6afbc837d7d192c925a1547b0e03ce326 Mon Sep 17 00:00:00 2001 From: sourcegraph-release-guild-bot <107104610+sourcegraph-release-guild-bot@users.noreply.github.com> Date: Wed, 10 Jan 2024 15:20:46 -0500 Subject: [PATCH] [Backport 5.2] Fix data access race condition in gRPC retry logic (#59488) Fix data access race condition in gRPC retry logic (#59487) (cherry picked from commit adb95ea5bcf7e3f9361c62153ef850923b41a1a4) Co-authored-by: Erik Seliger --- internal/grpc/retry/options.go | 5 +---- internal/grpc/retry/retry.go | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/grpc/retry/options.go b/internal/grpc/retry/options.go index 96f02bf752d0..53446634aa53 100644 --- a/internal/grpc/retry/options.go +++ b/internal/grpc/retry/options.go @@ -116,10 +116,7 @@ type CallOption struct { applyFunc func(opt *options) } -func reuseOrNewWithCallOptions(opt *options, callOptions []CallOption) *options { - if len(callOptions) == 0 { - return opt - } +func newWithCallOptions(opt *options, callOptions []CallOption) *options { optCopy := &options{} *optCopy = *opt for _, f := range callOptions { diff --git a/internal/grpc/retry/retry.go b/internal/grpc/retry/retry.go index 66fdc4106a0b..b778d2fe6cba 100644 --- a/internal/grpc/retry/retry.go +++ b/internal/grpc/retry/retry.go @@ -38,7 +38,7 @@ const ( // The default configuration of the interceptor is to not retry *at all*. This behaviour can be // changed through options (e.g. WithMax) on creation of the interceptor or on call (through grpc.CallOptions). func UnaryClientInterceptor(logger log.Logger, optFuncs ...CallOption) grpc.UnaryClientInterceptor { - intOpts := reuseOrNewWithCallOptions(defaultOptions, optFuncs) + intOpts := newWithCallOptions(defaultOptions, optFuncs) return func(parentCtx context.Context, fullMethod string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { tr := trace.FromContext(parentCtx) tr.SetAttributes(attribute.Bool(retriedTraceAttributeKey, false)) @@ -46,7 +46,7 @@ func UnaryClientInterceptor(logger log.Logger, optFuncs ...CallOption) grpc.Unar service, method := grpcutil.SplitMethodName(fullMethod) grpcOpts, retryOpts := filterCallOptions(opts) - callOpts := reuseOrNewWithCallOptions(intOpts, retryOpts) + callOpts := newWithCallOptions(intOpts, retryOpts) doTrace := makeTracingCallback(parentCtx, logger, service, method) originalCallback := callOpts.onRetryCallback @@ -104,7 +104,7 @@ func UnaryClientInterceptor(logger log.Logger, optFuncs ...CallOption) grpc.Unar // to buffer the messages sent by the client. If retry is enabled on any other streams (ClientStreams, // BidiStreams), the retry interceptor will fail the call. func StreamClientInterceptor(logger log.Logger, optFuncs ...CallOption) grpc.StreamClientInterceptor { - intOpts := reuseOrNewWithCallOptions(defaultOptions, optFuncs) + intOpts := newWithCallOptions(defaultOptions, optFuncs) return func(parentCtx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, fullMethod string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) { tr := trace.FromContext(parentCtx) tr.SetAttributes(attribute.Bool(retriedTraceAttributeKey, false)) @@ -112,7 +112,7 @@ func StreamClientInterceptor(logger log.Logger, optFuncs ...CallOption) grpc.Str service, method := grpcutil.SplitMethodName(fullMethod) grpcOpts, retryOpts := filterCallOptions(opts) - callOpts := reuseOrNewWithCallOptions(intOpts, retryOpts) + callOpts := newWithCallOptions(intOpts, retryOpts) // short circuit for simplicity, and avoiding allocations. doTrace := makeTracingCallback(parentCtx, logger, service, method)