grpc_retry.StreamClientInterceptor does not pass the correct "x-retry-attempty" metadata value

[slnt]

In retry.go in the implementation of StreamClientInterceptor there is the following code:

for attempt := uint(0); attempt < callOpts.max; attempt++ {
if err := waitRetryBackoff(attempt, parentCtx, callOpts); err != nil {
	return nil, err
}
callCtx := perCallContext(parentCtx, callOpts, 0)

This code should be (unless I'm missing some reason for it not to):

for attempt := uint(0); attempt < callOpts.max; attempt++ {
if err := waitRetryBackoff(attempt, parentCtx, callOpts); err != nil {
	return nil, err
}
callCtx := perCallContext(parentCtx, callOpts, attempt)

Also, there seems to be an extraneous y character in the definition of AttemptMetadataKey:

const (
	AttemptMetadataKey = "x-retry-attempty"
)

[johanbrandhorst]

Hi, thanks for the issue. It appears at least the AttemptMetadataKey typo is also present in v2, though the major issue here does appear to be addressed:

go-grpc-middleware/interceptors/retry/retry.go

Lines 30 to 70 in a5b9e0b

	func UnaryClientInterceptor(optFuncs ...CallOption) grpc.UnaryClientInterceptor {
	intOpts := reuseOrNewWithCallOptions(defaultOptions, optFuncs)
	return func(parentCtx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
	grpcOpts, retryOpts := filterCallOptions(opts)
	callOpts := reuseOrNewWithCallOptions(intOpts, retryOpts)
	// short circuit for simplicity, and avoiding allocations.
	if callOpts.max == 0 {
	return invoker(parentCtx, method, req, reply, cc, grpcOpts...)
	}
	var lastErr error
	for attempt := uint(0); attempt < callOpts.max; attempt++ {
	if err := waitRetryBackoff(attempt, parentCtx, callOpts); err != nil {
	return err
	}
	callCtx, cancel := perCallContext(parentCtx, callOpts, attempt)
	defer cancel() // Clean up potential resources.
	lastErr = invoker(callCtx, method, req, reply, cc, grpcOpts...)
	// TODO(mwitkow): Maybe dial and transport errors should be retriable?
	if lastErr == nil {
	return nil
	}
	callOpts.onRetryCallback(parentCtx, attempt, lastErr)
	if isContextError(lastErr) {
	if parentCtx.Err() != nil {
	logTrace(parentCtx, "grpc_retry attempt: %d, parent context error: %v", attempt, parentCtx.Err())
	// its the parent context deadline or cancellation.
	return lastErr
	} else if callOpts.perCallTimeout != 0 {
	// We have set a perCallTimeout in the retry middleware, which would result in a context error if
	// the deadline was exceeded, in which case try again.
	logTrace(parentCtx, "grpc_retry attempt: %d, context error from retry call", attempt)
	continue
	}
	}
	if !isRetriable(lastErr, callOpts) {
	return lastErr
	}
	}
	return lastErr
	}
	}

.

[bwplotka]

Typo fixed in #543