[Proposal] Should we keep or remove the non-generic ResilienceStrategy

[martintmk]

Recent enhancements to ResilienceStrategy<T> (#1428) have significantly improved the usability of generic resilience strategies, expanding use-case scenarios:

ResilienceStrategy<object> strategy = NullResilienceStrategy<object>.Instance;

int v1 = strategy.Execute(() => 10);
string v2 = strategy.Execute(() => "val");
bool v3 = strategy.Execute(() => true);

This progress paves the way for potential removal of the non-generic ResilienceStrategy type and associated infrastructure. This proposal brings both advantages and disadvantages that should be thoroughly assessed before making a decision.

Advantages

1. Reduced API Surface: This major advantage simplifies maintenance efforts.
2. Inherent Support for Reactive Strategies: Currently, reactive strategies utilize OutcomeResilienceStrategy as a base class, bridging ResilienceStrategy to ResilienceStrategy<T>. This change would render the bridge class obsolete, integrating support for reactive strategies directly.

Disadvantages

1. Complex Strategy Composition: The existing resilience strategy implements ResilienceStrategy, which lacks a generic constraint, facilitating strategy composition. If non-generic strategies are removed, the new ResilienceStrategy<T> signature would restrict composability to strategies of T or ResilienceStrategy<object>.
2. Imposed Constraints on Non-Reactive Strategies: Strategies such as Timeout and RateLimiter are indifferent to result types and can naturally be executed across all result types. Under the new model, these strategies would no longer have this "natural implementation."
3. Lack of Inherent Support for Void Result Types: The removal of ResilienceStrategy would eliminate a straightforward method for executing void-based callbacks. While a VoidResult type could be introduced, usage would be more complex than the existing system.
4. Complicated Usage of ResilienceStrategy: Presently, it's simple to pass and flow ResilienceStrategy in the API for basic scenarios or when exceptions are exclusively handled by the consumer. In the proposed model, passing ResilienceStrategy<object> feels less intuitive.

Current Usage:

ResilienceStrategy strategy = new ResilienceStrategyBuilder()
    .AddTimeout(TimeSpan.FromSeconds(10))
    .AddRetry(new RetryStrategyOptions
    {
        RetryCount = 4,
        BaseDelay = TimeSpan.FromSeconds(10),
    })
    .Build();

ResilienceStrategyRegistry<string> registry = new ResilienceStrategyRegistry<string>();
ResilienceStrategy strategyFromRegistry = registry.GetStrategy("my-strategy");

strategy.Execute(() => "dummy");
strategy.Execute(() => 10);
strategy.Execute(() => { SomeCall(); });

New usage:

ResilienceStrategy<object> strategy = new ResilienceStrategyBuilder<object>()
    .AddTimeout(TimeSpan.FromSeconds(10))
    .AddRetry(new RetryStrategyOptions<object>
    {
        RetryCount = 4,
        BaseDelay = TimeSpan.FromSeconds(10),
    })
    .Build();

ResilienceStrategyRegistry<string> registry = new ResilienceStrategyRegistry<string>();
ResilienceStrategy<object> strategyFromRegistry = registry.GetStrategy<object>("my-strategy");

strategy.Execute(() => "dummy");
strategy.Execute(() => 10);
strategy.Execute(() => { SomeCall(); return VoidResult.Instance; });

Contributes to #1365

Would love your feedback and insights on this matter before we proceed with any modifications, as these changes could significantly affect the usability of V8.

@martincostello
@joelhulen
@juraj-blazek
@geeknoid
@PeterCsalaHbo
@andrey-noskov
@vany0114

[martincostello]

At a summary glance, the proposed changes look a lot clunkier to use.

[andrey-noskov]

I see almost no difference in the usage though I see a lot of benefits
so, this "Complex Strategy Composition" imo is an advantage that should be treated as "now the compiler can analyze the composition tree at build time"

[andrey-noskov]

Imposed Constraints on Non-Reactive Strategies:

I couldn't catch this - is there a specific use-case? why would anyone want to re-use a strategy across different pipelines?

[andrey-noskov]

Lack of Inherent Support for Void Result Types:

this can be hidden behind a syntax sugar Execute<Action<..>> method, right?

[martintmk]

I see almost no difference in the usage though I see a lot of benefits so, this "Complex Strategy Composition" imo is an advantage that should be treated as "now the compiler can analyze the composition tree at build time"

That's also the way it works currently. Generic builder won't allow you to combine strategies that work with different result types while still allowing to add non-reactive strategies to it. Although that check is only when using public APIs. Internally the core indeed uses non-generic strategies for everything.

Lack of Inherent Support for Void Result Types:

this can be hidden behind a syntax sugar Execute<Action<..>> method, right?

Maybe to expose these as void-based extensions for ResilienceStrategy<object>, but it feels hacky to me.

I couldn't catch this - is there a specific use-case? why would anyone want to re-use a strategy across different pipelines?

For example, shared circuit breaker across multiple pipelines. Ech pipeline handles different result types, but uses non-generic circuit breaker.

see #959 or @martincostello project that mirrors their internal project:
https://github.com/martincostello/polly-sandbox/blob/90636486a54b621ff58289dec32e1c7bc78e9f7e/src/PollySandbox/ResilienceStrategyFactory.cs#L46

[vany0114]

Seems like there are more cons than pros, while it favors the maintenance I think it disfavors the usage experience, personally, it feels kinda convoluted, especially for the void strategies.

[adamnova]

I agree that unifying the builders would improve maintainability. However, the new usage looks to me like an API limitation workaround. I believe that object should be used sparingly in a type safe language and with this change it would likely be introduced to many use cases that do not care about the result. All this adds to complexity and makes Polly more difficult to use for anyone who just picks it up.

To me generic/non-generic is the intuitive way to do it. If I care about a result, I specify the type. If I don't I don't specify anything. The unification would be fine, if C# had standard object type for null/void/no data. But instead it treats void special, so I think Polly should do it the same way, not try to be different.

Ease of use is probably the most important thing. If it's too difficult, people would rather write it themselves, usually all they need is just retries and you can do that fairly easily.

[martincostello]

I agree with the comment above. The API surface as it is today with regards to TResult vs void looks more intuitive to use than having to use <object> (or <VoidType> etc.) to force a type so that everything is a generic strategy.

I think we should err on the side of optimising the API for the consumers' usage (within reason), rather than making it harder to the use for the majority for the benefit of a minority (i.e. the Polly maintainers and contributors).

Polly is a very popular package and the spectrum of knowledge/experience its users have ranges from the novice to the deep expert. The majority of the developers using Polly will be on the less experienced side of that spectrum, so I think hampering the usability of the API to try and remove the long tail of remaining potentially optimisable CPU cycles out of the design where it affects the end users is something we should avoid.

[martintmk]

Thank everyone for the feedback!

I would like to add more context on why the core building block, ResilienceStrategy, doesn't have a generic constraint.

As we designed the V8 API, we knew we would implement two types of strategies:

1. Reactive strategies.
2. Non-Reactive strategies.

The reactive strategies inspect and handle concrete result types. Hence, when defining them, consumers must provide generic callbacks to manage the values. On the other hand, the non-reactive strategies only perform actions before or after the execution callback provided by the user. They're indifferent to the result type.

Moreover, it was common to combine reactive strategies with non-reactive ones.

If we were to place a generic constraint on ResilienceStrategy, it would impact the non-reactive strategies, which would then be constrained to a specific type. Additionally, this would complicate the composition of reactive and non-reactive strategies, leading to the need for workarounds, such as representing the shared strategies as ResilienceStrategy<object>.

A challenge we faced was exposing the API for both generic and non-generic strategies without causing excessive code duplication. For non-reactive strategies, avoiding duplication was relatively straightforward, as they function equally well with both generic and non-generic APIs.

In the case of reactive strategies, we underwent several iterations, each one progressively reducing the amount of code, until we finally settled on the current model.

Each reactive strategy now defines generic options:

public class RetryStrategyOptions<T>
{
   // omitted for brevity
}

For non-generic APIs, we simply expose non-generic options derived from the generic ones, thereby minimizing duplication:

public class RetryStrategyOptions : RetryStrategyOptions<object>
{
}

In terms of implementation, a single one exists for both generic and non-generic versions, eliminating any duplication. An example of this can be found in RetryResilienceStrategy. Reactive strategies use a straightforward bridge that adds a generic constraint.

Lastly, the strategy needs to be exposed on both CompositeStrategyBuilder and CompositeStrategyBuilder<T>. This part requires a bit of duplication, as two extensions must be implemented. However, both extensions are straightforward and call the same helper method. See RetryCompositeStrategyBuilderExtensions for more information.

While it's possible that extension authors may unintentionally mix unsupported reactive strategies, this issue can easily be spotted through unit testing. For the typical consumer, the API is safe and doesn't permit the mixing of incompatible strategies, such as combining ResilienceStrategy<string> with ResilienceStrategy<int>.

I hope this explanation clarifies and provides more details on the V8 design.

[joelhulen]

I agree with the others here who believe that removing non-generic strategies has more cons than pros, not least of which is adding complexity where there is no clear benefit in doing so for most developers/use cases. Explicitly using object and introducing a VoidResult type seems hacky and would give our API a bad sense of code smell. I do not think that we should remove the non-generic types.

[laura-mi]

I think we should keep both the non-generic & generic support. It is a bit of more code on our side but it is for the benefit of users and usage simplicity, which seems worth.

The usage of one policy for various possible return types as well as void "returns" is a common use case. Forcing a meta type which allows both "object" and "void" looks like a hacky workaround to me as Joel mentioned.

For the usage of policies for handling exceptions only (assuming any result a valid object to return back) there is no need to declare a typed policy as the return type is not relevant. However users would need to force some "dummy" type which will only clutter the method for no benefit.

[joelhulen]

We are making the executive decision to keep both non-generic and generic support. Marking issue as closed.