feat(retry): add configuration to support disable timeout retry when …

…do failure retry, which is for the non-idempotent request (#887)

client/client_test.go

@@ -544,15 +544,24 @@ func TestRetryWithResultRetry(t *testing.T) {
 
 	mockErr := errors.New("mock")
 	retryWithMockErr := false
-	var count int32
 	errMW := func(next endpoint.Endpoint) endpoint.Endpoint {
+		var count int32
 		return func(ctx context.Context, req, resp interface{}) (err error) {
 			if atomic.CompareAndSwapInt32(&count, 0, 1) {
 				return mockErr
 			}
 			return nil
 		}
 	}
+	mockTimeoutMW := func(next endpoint.Endpoint) endpoint.Endpoint {
+		var count int32
+		return func(ctx context.Context, req, resp interface{}) (err error) {
+			if atomic.CompareAndSwapInt32(&count, 0, 1) {
+				time.Sleep(300 * time.Millisecond)
+			}
+			return nil
+		}
+	}
 	errRetryFunc := func(err error, ri rpcinfo.RPCInfo) bool {
 		if errors.Is(err, mockErr) {
 			retryWithMockErr = true
@@ -561,7 +570,7 @@ func TestRetryWithResultRetry(t *testing.T) {
 		return false
 	}
 
-	// should timeout
+	// case 1: retry for mockErr
 	cli := newMockClient(t, ctrl,
 		WithMiddleware(errMW),
 		WithRPCTimeout(100*time.Millisecond),
@@ -580,6 +589,40 @@ func TestRetryWithResultRetry(t *testing.T) {
 	err := cli.Call(context.Background(), mtd, req, res)
 	test.Assert(t, err == nil, err)
 	test.Assert(t, retryWithMockErr)
+
+	// case 1: retry for timeout
+	cli = newMockClient(t, ctrl,
+		WithMiddleware(mockTimeoutMW),
+		WithRPCTimeout(100*time.Millisecond),
+		WithFailureRetry(&retry.FailurePolicy{
+			StopPolicy: retry.StopPolicy{
+				MaxRetryTimes: 3,
+				CBPolicy: retry.CBPolicy{
+					ErrorRate: 0.1,
+				},
+			},
+			RetrySameNode:     true,
+			ShouldResultRetry: &retry.ShouldResultRetry{ErrorRetry: errRetryFunc},
+		}))
+	err = cli.Call(context.Background(), mtd, req, res)
+	test.Assert(t, err == nil, err)
+
+	// case 2: set NotRetryForTimeout as true, won't do retry for timeout
+	cli = newMockClient(t, ctrl,
+		WithMiddleware(mockTimeoutMW),
+		WithRPCTimeout(100*time.Millisecond),
+		WithFailureRetry(&retry.FailurePolicy{
+			StopPolicy: retry.StopPolicy{
+				MaxRetryTimes: 3,
+				CBPolicy: retry.CBPolicy{
+					ErrorRate: 0.1,
+				},
+			},
+			RetrySameNode:     true,
+			ShouldResultRetry: &retry.ShouldResultRetry{ErrorRetry: errRetryFunc, NotRetryForTimeout: true},
+		}))
+	err = cli.Call(context.Background(), mtd, req, res)
+	test.Assert(t, errors.Is(err, kerrors.ErrRPCTimeout))
 }
 
 func TestFallbackForError(t *testing.T) {

pkg/retry/failure_retryer.go

@@ -219,7 +219,7 @@ func (r *failureRetryer) isRetryErr(err error, ri rpcinfo.RPCInfo) bool {
 	// But CircuitBreak has been checked in ShouldRetry, it doesn't need to filter ServiceCircuitBreak.
 	// If there are some other specified errors that cannot be retried, it should be filtered here.
 
-	if kerrors.IsTimeoutError(err) {
+	if r.policy.IsRetryForTimeout() && kerrors.IsTimeoutError(err) {
 		return true
 	}
 	if r.policy.IsErrorRetryNonNil() && r.policy.ShouldResultRetry.ErrorRetry(err, ri) {

pkg/retry/policy.go

@@ -90,20 +90,19 @@ type FailurePolicy struct {
 	ShouldResultRetry *ShouldResultRetry `json:"-"`
 }
 
-// IsRespRetryNonNil is used to judge if RespRetry is nil
+// IsRespRetryNonNil is used to check if RespRetry is nil
 func (p FailurePolicy) IsRespRetryNonNil() bool {
-	if p.ShouldResultRetry != nil && p.ShouldResultRetry.RespRetry != nil {
-		return true
-	}
-	return false
+	return p.ShouldResultRetry != nil && p.ShouldResultRetry.RespRetry != nil
 }
 
-// IsErrorRetryNonNil is used to judge if ErrorRetry is nil
+// IsErrorRetryNonNil is used to check if ErrorRetry is nil
 func (p FailurePolicy) IsErrorRetryNonNil() bool {
-	if p.ShouldResultRetry != nil && p.ShouldResultRetry.ErrorRetry != nil {
-		return true
-	}
-	return false
+	return p.ShouldResultRetry != nil && p.ShouldResultRetry.ErrorRetry != nil
+}
+
+// IsRetryForTimeout is used to check if timeout error need to retry
+func (p FailurePolicy) IsRetryForTimeout() bool {
+	return p.ShouldResultRetry == nil || !p.ShouldResultRetry.NotRetryForTimeout
 }
 
 // BackupPolicy for backup request
@@ -166,6 +165,8 @@ const (
 type ShouldResultRetry struct {
 	ErrorRetry func(err error, ri rpcinfo.RPCInfo) bool
 	RespRetry  func(resp interface{}, ri rpcinfo.RPCInfo) bool
+	// disable the default timeout retry in specific scenarios (e.g. the requests are not non-idempotent)
+	NotRetryForTimeout bool
 }
 
 // Equals to check if policy is equal

pkg/retry/policy_test.go

@@ -406,6 +406,31 @@ func TestPolicyNotEqual(t *testing.T) {
 	test.Assert(t, !p.Equals(policy))
 }
 
+func TestPolicyNotRetryForTimeout(t *testing.T) {
+	// create failurePolicy
+	fp := &FailurePolicy{StopPolicy: StopPolicy{
+		MaxRetryTimes:    1,
+		MaxDurationMS:    2,
+		DisableChainStop: false,
+		DDLStop:          false,
+		CBPolicy: CBPolicy{
+			ErrorRate: defaultCBErrRate,
+		},
+	}}
+	// case 1: ShouldResultRetry is nil, retry for timeout
+	test.Assert(t, fp.IsRetryForTimeout())
+
+	// case 2: ShouldResultRetry is not nil, NotRetryForTimeout is false, retry for timeout
+	fp.ShouldResultRetry = &ShouldResultRetry{
+		ErrorRetry: nil,
+		RespRetry:  nil,
+	}
+
+	// case 3: ShouldResultRetry is not nil, NotRetryForTimeout is true, not retry for timeout
+	fp.ShouldResultRetry.NotRetryForTimeout = true
+	test.Assert(t, !fp.IsRetryForTimeout())
+}
+
 func genRPCInfo() rpcinfo.RPCInfo {
 	to := remoteinfo.NewRemoteInfo(&rpcinfo.EndpointBasicInfo{Method: method}, method).ImmutableView()
 	ri := rpcinfo.NewRPCInfo(to, to, rpcinfo.NewInvocation("", method), rpcinfo.NewRPCConfig(), rpcinfo.NewRPCStats())