Call retry callback on retry (#700)

* Move on retry callback to when it retries

* the same for streaming

* after backoff wait

* add test on retry callback on non-retriable errors

Author: fredr

interceptors/retry/retry.go

@@ -40,14 +40,16 @@ func UnaryClientInterceptor(optFuncs ...CallOption) grpc.UnaryClientInterceptor
 			if err := waitRetryBackoff(attempt, parentCtx, callOpts); err != nil {
 				return err
 			}
+			if attempt > 0 {
+				callOpts.onRetryCallback(parentCtx, attempt, lastErr)
+			}
 			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())
@@ -94,6 +96,9 @@ func StreamClientInterceptor(optFuncs ...CallOption) grpc.StreamClientIntercepto
 			if err := waitRetryBackoff(attempt, parentCtx, callOpts); err != nil {
 				return nil, err
 			}
+			if attempt > 0 {
+				callOpts.onRetryCallback(parentCtx, attempt, lastErr)
+			}
 			var newStreamer grpc.ClientStream
 			newStreamer, lastErr = streamer(parentCtx, desc, cc, method, grpcOpts...)
 			if lastErr == nil {
@@ -107,7 +112,6 @@ func StreamClientInterceptor(optFuncs ...CallOption) grpc.StreamClientIntercepto
 				}
 				return retryingStreamer, 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())

interceptors/retry/retry_test.go

@@ -204,6 +204,21 @@ func (s *RetrySuite) TestUnary_OnRetryCallbackCalled() {
 	require.EqualValues(s.T(), 2, retryCallbackCount, "two retry callbacks should be called")
 }
 
+func (s *RetrySuite) TestUnary_OnRetryCallbackNotCalledOnNonRetriableError() {
+	retryCallbackCount := 0
+
+	s.srv.resetFailingConfiguration(3, codes.Internal, noSleep) // see retriable_errors
+	out, err := s.Client.Ping(s.SimpleCtx(), testpb.GoodPing,
+		WithOnRetryCallback(func(ctx context.Context, attempt uint, err error) {
+			retryCallbackCount++
+		}),
+	)
+
+	require.Error(s.T(), err, "should result in an error")
+	require.Nil(s.T(), out, "out should be nil")
+	require.EqualValues(s.T(), 0, retryCallbackCount, "no retry callbacks should be called")
+}
+
 func (s *RetrySuite) TestServerStream_SucceedsOnRetriableError() {
 	s.srv.resetFailingConfiguration(3, codes.DataLoss, noSleep) // see retriable_errors
 	stream, err := s.Client.PingList(s.SimpleCtx(), testpb.GoodPingList)