[Feature request]: allow MaxRetryAttempts=0

[verdie-g]

Is your feature request related to a specific problem? Or an existing feature?

I'm migrating from policies to resilience pipelines and I saw many of my tests failing because of

System.ComponentModel.DataAnnotations.ValidationException : The 'RetryStrategyOptions<HttpResponseMessage>' are invalid.
  Validation Errors:
    The field MaxRetryAttempts must be between 1 and 2147483647.

It seems like 0 is not accepted anymore.

The work around is to do

var pipeline = new ResiliencePipelineBuilder();
if (retryCount > 0)
{
    pipeline.AddRetry(new RetryStrategyOptions
    {
          ShouldHandle = new PredicateBuilder().Handle<Exception>(),
          MaxRetryAttempts = retryCount,
    });
}
pipeline.Build();

but it's kind of easy to miss.

In my case, the MaxRetryAttempts can come from dynamic configurations. So, changing a config to disable the pipeline by setting 0 to MaxRetryAttemps could actually break the application.

Describe the solution you'd like

When MaxRetryAttemps is set to 0, it would simply disable the retry.

Additional context

No response

[martincostello]

I would argue allowing this could have the opposite effect of allowing you to accidentally configure a non-functional retry policy while still paying for the performance overhead of a retry that will never actually retry.

I think the change for v8 makes more overall sense (retrying 0 times isn't a retry), so the correct thing to do is your "workaround" of not adding a policy that doesn't make any sense in the first place.

Either way has an advantage and a disadvantage, and I think making mis-configuration apparent/obvious is better than silent failure (i.e. no retries).

[verdie-g]

Yeah I'm fine with that idea. Just wanted to open the discussion because I'm pretty sure we'll have an incident in the future where a retry config is set to 0.

[martincostello]

I'm glad you test your policies though 😄

I've always maintained people should configure their policies and then test they behave the way they think they've configured them is correct.

[kmcclellan]

I too would like to see zero attempts allowed and interpreted as effectively disabling the strategy.

Consider that application topology differs from application configuration. When I build a resilience pipeline, certain assumptions are codified and are less easily changed (viz. what strategies exist and in what order they execute). On the other hand, given the options pattern, the pipeline supports a number of different possible configurations. Whether or not a given strategy is enabled should be a matter of configuration IMO and should not require changes to the topology of the pipeline.

[github-actions[bot]]

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

[github-actions[bot]]

This issue was closed because it has been inactive for 14 days since being marked as stale.