Polly: Cleanly formalise the separation of sync and async policies (was: option to unify them)

[reisenberger]

TL;DR At the expense of some syntax changes (which would eventually need to be pushed through as breaking changes), we could unify sync and async policies in Polly. What do users think?

A new syntax such as below would be a breaking change, but would allow sync and async policies to be unified:

```c#
.WaitAndRetry(int retryCount, Func sleepDurationProvider)
.OnRetry(Action onRetry) // optional postfix configuration step
.OnRetryAsync(Func onRetryAsync) // optional postfix configuration step

## Background: Current sync/async split 

With Polly we currently must use separate Polly policies for synchronous and asynchronous executions:  

+ When you define a policy, you define it as either (eg) `Retry()` or `RetryAsync()`
+ You can only `Execute()` (etc) on synchronous policies; `ExecuteAsync()` on async policies; but [not use one policy instance for both sync and async executions](https://github.com/App-vNext/Polly/blob/cfef09907d45cb82846e03e6a11b6a4f6d51abfe/src/Polly.Shared/PolicyAsync.cs#L1126-L1127).

That architecture evolved due to historic decisions (before current maintainers), and [this blog post](http://www.thepollyproject.org/2017/06/09/polly-and-synchronous-versus-asynchronous-policies/) explored some of the reasons and blocks to changing this - to unifying sync and async policies.  

Decisive is that users expect async policies to have async state-change delegates, while sync policies must have sync ones.  So a policy that would act for both sync and async executions, must at least _allow_ the definition of all state-change delegates in both sync and async forms.

## The current syntax - a block to change? 

A block to defining policies with both/either sync and async state-change delegates is Polly's current syntax.  Take one of the fuller `WaitAndRetry()` configuration overloads:

[`.WaitAndRetry(int retryCount, Func<int, Context, TimeSpan> sleepDurationProvider, Action<Exception, TimeSpan, int, Context> onRetry)`](
https://github.com/App-vNext/Polly/blob/cfef09907d45cb82846e03e6a11b6a4f6d51abfe/src/Polly.Shared/Retry/RetrySyntax.cs#L326).  

Add the async state-change delegate, and you have the slightly clumsy: 

`.WaitAndRetry(int retryCount, Func<int, Context, TimeSpan> sleepDurationProvider, Action<Exception, TimeSpan, int, Context> onRetry, Func<Exception, TimeSpan, int, Context, Task> onRetryAsync)`

With policies that take a greater number of state-change delegates (circuit-breaker's `onBreak`, `onReset` and `onHalfOpen`), overloads quickly become very ugly.  And all in addition to existing overloads.

## The current syntax - good and bad

The driving factor in the current syntax is that, beyond the `.Handle<>()` predicates, everything about a policy must be configured in one single overload call.  

_Negatives_:

+ It leads to the current proliferation of configuration overloads.
+ It counts against adding more overloads (therefore more features).
+ It makes overloads for mixed sync/async policies clumsy.  

_Positives_:

+ Configuring everything-in-one-shot means policies are immutable.  Immutability is generally good: here, it prevents bugs arising where one part of the code might accidentally change a policy that another part of the code is already using.

## New syntax could offer progress?

### ... (at the expense of breaking changes) 

An alternative syntax could keep all the primary characteristics of a policy configured in one shot (they often operate as a unit), while allowing state-change delegates to be configured by fluent postfix:

```c#
.WaitAndRetry(int retryCount, Func<int, Context, TimeSpan> sleepDurationProvider)
    .OnRetry(Action<Exception, TimeSpan, int, Context> onRetry) // optional postfix configuration step
    .OnRetryAsync(Func<Exception, TimeSpan, int, Context, Task> onRetryAsync) // optional postfix configuration step

This:

• drastically reduces the overall number of overloads
• allows adding both sync and async state-change delegates without creating unwieldy method signatures.

How would state-change delegates on such a policy operate?

If both .OnRetry() and .OnRetryAsync() were configured, a sync execution would use the sync onRetry, an async execution the onRetryAsync().

If only .OnRetry() (sync form) were configured, what should an _async_ execution through the policy do? It could:

(a) invoke no onRetry; or
(b) invoke Task.FromResult(onRetry(...)) (or (C#7), with ValueTask) (_current preferred solution_)

Choice (b) offers some convenience: only one on-retry overload may be configured, but the policy _could_ still be used in both sync and async cases with that on-retry.

If only .OnRetryAsync() were configured, what should a _sync_ execution through the policy do? We _cannot_ invoke an async method and block on it with .Result; we will not introduce Polly blocking on async code. We could:

(c) silently invoke no on-retry.

Or (d) throw. (_current preferred solution_: The user might assume the configured onRetryAsync() would be invoked, but it cannot; we should better signal that, with an exception, than silently drop behaviour).

_Note_ that (b) above (whether with (c) or (d)) gives _asymmetric behaviour_: async-execution-when-only-sync-state-change-defined would use the state-change delegate, but sync-execution-when-only-async-state-change-defined would omit-state-change-delegate or throw. This asymmetry is probably a price worth paying.

How could we preserve immutability, in the new syntax?

A positive of the existing syntax is policy (/state-change-delegate) immutability. That could still be enforced by:

• prevent a state-change-delegate being modified if one has already been configured; _or_
• prevent a state-change-delegate being modified if any execution has already taken place.

Applicability

While this discussion focuses on retry, the changes should be made across _all_ policy types if implemented. This represents a reasonable amount of work.

User feedback wanted

• Would Polly users like to see sync/async policies unified in this manner, albeit with a change of syntax?
  
  
  
  • Old overloads could remain included, marked deprecated, for some releases; but eventually they would need removing, as a breaking API change.
  
  
• Should policy (state-change-delegate) immutability be enforced?
• Other ideas for syntax? Other thoughts?

[Im5tu]

I would definitely agree that uniformity is a good thing between the api's, I suggest the approach:

1 - Add in the new apis (in vnext)
2 - Mark the existing apis as obsolete, linking to this discussion and/or a fix (in vnext)
3 - Remove the obsolete apis post vnext

This gives users some time to transition and makes the experience better overall. I wouldn't keep the old API around for very long as it will hamper productivity of new features etc.

+1 for immutability being enforced

[reisenberger]

Thanks @Im5tu for joining the Polly conversation!

Re procedure, yes, that's exactly what we would expect to follow (per the Semver recommended practice).

Good call.

[reisenberger]

Noting herewith some responses via twitter: https://twitter.com/softwarereisen/status/889939361298567172

[dannydwarren]

I always wished I had one policy for both sync and async. And agree with @Im5tu's points. Also, based on experience this would help with the learning curve.

[brianfeucht]

Is it possible to split these apart so that you can enforce that async policies are used with the ExecuteAsync method at compile time vs execution time?

Seems like this approach would create only breaking changes in places where execution would fail at run time anyways. Trying to unify the two together seems like it will be a pretty big code change for anyone upgrading.

I'm 100% for breaking changes if it means I can get feedback about API miss use at compile time vs run time.

[reisenberger]

Hi @brianfeucht . Great questions. Re:

Is it possible to split these apart so that you can enforce that async policies are used
with the ExecuteAsync method at compile time vs execution time?

Yeah, the original runtime-only (not compile-time) enforcement of async-policies for async-executions is a negative. Came in before my time on the project, but we've recently added (in a non-breaking way) what should give you the compile-time enforcement you're after. v5.2.0 added interfaces which separate sync and async. So right now, for example, you can declare a policy as, eg:

ISyncPolicy policy = Policy.Handle<Exception>().Retry(3);

or:

IAsyncPolicy policy = Policy.Handle<Exception>().RetryAsync(3);

(and similarly for TResult-typed policies ISyncPolicy<TResult> and IAsyncPolicy<TResult>). You can only .ExecuteAsync(...) (or .ExecuteAndCaptureAsync(...) ) on an IAsyncPolicy etc.

[ohadschn]

@reisenberger remembering to do ISyncPolicy policy = Policy.Handle<Exception>().Retry(3); is just like remembering not to do ISyncPolicy policy = Policy.Handle<Exception>().Retry(3).ExecuteAsync(...).

Worse, there is nothing stopping me from doing IAsyncPolicy policy = Policy.Handle<Exception>().Retry(3). IMHO having Policy implement both ISyncPolicy and IAsyncPolicy does more harm than good.

A true compiler guarantee would have Retry and RetryAsync returning different and distinct interfaces, so you couldn't make such a mistake even if you wanted to.

[reisenberger]

_Triage of old issues_ (@ohadschn, apologies for the delayed reply). Re:

A true compiler guarantee would have Retry and RetryAsync returning different and distinct interfaces

Yes, that's entirely correct. If we retain the sync/async split in Polly, this is what we should do.

The reason I did not do this is that it would drive the wedge of the sync/async split deeper into Polly (and force a breaking change on users while doing so), when the intention of this work item is to remove it (unify sync/async). I didn't want to push users in the direction of one breaking change (returning the interface instead of concrete Policy type would be a breaking change for many), only to push them in the _opposite_ direction with another breaking change (unifying sync/async) thereafter.

It's definitely true to say though that this leaves us - somewhat unsatisfactorily, as you say - only part-way through clearing up the runtime sync/async split inherited when I took over the project. In the meantime, the availability of the interfaces does at least allow users an option to enforce compile-time failure (without breaking changes) if they want to.

[ohadschn]

@reisenberger understood, thank you for explaining. Perhaps something to consider for the next major version (in which breaking changes are fair play).

[reisenberger]

_Triage_: further consideration of the sync/async split will sit behind getting enough of #326 in place, that policies are emitting events which can be aggregated to metrics

[dustyhoppe]

Re: They syntax proposal around merging async/sync support

.WaitAndRetry(int retryCount, Func<int, Context, TimeSpan> sleepDurationProvider)
    .OnRetry(Action<Exception, TimeSpan, int, Context> onRetry) // optional postfix configuration step
    .OnRetryAsync(Func<Exception, TimeSpan, int, Context, Task> onRetryAsync)

I'm a fan of this ^^ proposal.

Re: How would state-change delegates on such a policy operate?

(b) invoke Task.FromResult(onRetry(...)) (or (C#7), with ValueTask) (current preferred solution)

I'd like to add another vote to option (b).

[reisenberger]

Picking up this issue as part of work towards Polly v7.0.

From Polly v7.0 I am proposing we confirm and cleanly formalise separate sync and async policies. This recommendation is made after practical time investigating the feasibility and cleanliness of the unified sync-async approach (spiked for v6, revisited for v7).

On the surface unified sync-async policies seem appealing, but (as already discussed in thread), the simplicity unravels into complexity around the policy hooks. The story becomes [^]:

• You define a single policy for both sync and async usage, and you can define either or both sync or async policy hooks
• If you define both sync and async policy hooks (and: couldn't being able to define both lead to possible contradictions in behaviour also?), an async usage of the policy will prefer to use the async policy hook and ignore the sync policy hook; but can use the sync policy hook if that's the only one you define.
• If you define both sync and async policy hooks, a sync usage of the policy will use the sync one. But if you define only an async policy hook, a sync usage of that policy will (do what?) -> (it has to) throw, because you can't (cleanly) use an async delegate hook amid a sync execution ...

That story seems a lot more complicated than:

• For async executions define and use an async policy.
• For sync executions define and use a sync policy.

Further drivers were:

• Opening up custom policies: I was happy to sustain the complexity of implementation for [^] as lead developer for Polly's internal policies, but the complex story [^] is not one I want to sell to those implementing custom policies.
• The strong tie-in with .Net Core HttpClientFactory. This uses only policies in IAsyncPolicy<HttpResponseMessage>. If users wish to develop custom policies for that use case, there's no need for us to force them to implement also a sync implementation (just to satisfy a joint sync-async policy approach).

[reisenberger]

9c0a03e delivers the clean sync/async separation; intended for release in Polly v7.

[reisenberger]

Merged to v7.0.0 branch by #552. Change will be released when v7.0.0 is merged to master and released.

Impact of change clearly documented in the wiki

[johnknoop]

I think it would be nice to reduce coupling. An example:

This is how I set up a named HttpClient in .NET Core:

services.AddHttpClient(HttpClients.GeoCode)
    .AddTransientHttpErrorPolicy(x => x.RetryAsync(3));

I have to choose RetryAsync and not Retry because I happen to know that the part of my application that uses this HttpClient will call GetAsync. This to me is an unfortunate coupling that I would love to get rid of.

[reisenberger]

@johnknoop At present you have to know/remember to use .RetryAsync(...) not .Retry(...) because the original developers who added async to Polly made using a synchronous policy with an asynchronous call a runtime error, not a compile-time error. From Polly v7 (releasing shortly) we are changing this to be a compile-time error. You will simply not be able to code:

services.AddHttpClient(HttpClients.GeoCode)
.AddTransientHttpErrorPolicy(x => x.Retry(3));

because it will not compile. Only the form:

services.AddHttpClient(HttpClients.GeoCode)
.AddTransientHttpErrorPolicy(x => x.RetryAsync(3));

will compile.

[johnknoop]

@reisenberger Great!