-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanly formalise the separation of sync and async policies (was: option to unify them) #281
Comments
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) 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 |
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. |
Noting herewith some responses via twitter: https://twitter.com/softwarereisen/status/889939361298567172 |
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. |
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. |
Hi @brianfeucht . Great questions. Re:
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:
or:
(and similarly for TResult-typed policies |
@reisenberger remembering to do Worse, there is nothing stopping me from doing A true compiler guarantee would have |
Triage of old issues (@ohadschn, apologies for the delayed reply). Re:
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. |
@reisenberger understood, thank you for explaining. Perhaps something to consider for the next major version (in which breaking changes are fair play). |
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 |
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?
I'd like to add another vote to option (b). |
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 [^]:
That story seems a lot more complicated than:
Further drivers were:
|
9c0a03e delivers the clean sync/async separation; intended for release in Polly v7. |
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 |
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 |
@johnknoop At present you have to know/remember to use
because it will not compile. Only the form:
will compile. |
@reisenberger Great! |
This comment on #895 says "Polly v8 unifies sync and async policies into a single API concept, which should address this.". I don't have a link to any documentation, however. |
Please take a look at pollydocs.org, which is linked to in the README, for the documentation for the latest release of Polly and the migration guide from v7. |
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:
Background: Current sync/async split
With Polly we currently must use separate Polly policies for synchronous and asynchronous executions:
Retry()
orRetryAsync()
Execute()
(etc) on synchronous policies;ExecuteAsync()
on async policies; but not use one policy instance for both sync and async executions.That architecture evolved due to historic decisions (before current maintainers), and this blog post 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)
.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
andonHalfOpen
), 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:
Positives:
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:
This:
How would state-change delegates on such a policy operate?
If both
.OnRetry()
and.OnRetryAsync()
were configured, a sync execution would use the synconRetry
, an async execution theonRetryAsync()
.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), withValueTask
) (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:
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
The text was updated successfully, but these errors were encountered: