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

OPTION: Unify sync and async policies, with breaking syntax changes #281

Open
reisenberger opened this Issue Jul 25, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@reisenberger
Member

reisenberger commented Jul 25, 2017

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:

.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

Background: Current sync/async split

With Polly we currently must use separate Polly policies for synchronous and asynchronous 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 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:

.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?

@reisenberger reisenberger changed the title from PROPOSAL: Unify sync and async policies, with syntax changes to OPTION: Unify sync and async policies, with breaking syntax changes Jul 25, 2017

@Im5tu

This comment has been minimized.

Show comment
Hide comment
@Im5tu

Im5tu Jul 26, 2017

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

Im5tu commented Jul 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@reisenberger

reisenberger Jul 26, 2017

Member

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.

Member

reisenberger commented Jul 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@reisenberger
Member

reisenberger commented Jul 26, 2017

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

@dannydwarren

This comment has been minimized.

Show comment
Hide comment
@dannydwarren

dannydwarren Aug 2, 2017

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.

dannydwarren commented Aug 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@brianfeucht

brianfeucht Aug 17, 2017

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.

brianfeucht commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@reisenberger

reisenberger Aug 18, 2017

Member

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.

Member

reisenberger commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ohadschn

ohadschn Aug 29, 2017

@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.

ohadschn commented Aug 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@reisenberger

reisenberger Jan 2, 2018

Member

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.

Member

reisenberger commented Jan 2, 2018

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

This comment has been minimized.

Show comment
Hide comment
@ohadschn

ohadschn Jan 3, 2018

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

ohadschn commented Jan 3, 2018

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

@reisenberger

This comment has been minimized.

Show comment
Hide comment
@reisenberger

reisenberger Jan 6, 2018

Member

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

Member

reisenberger commented Jan 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@dustyhoppe

dustyhoppe Mar 31, 2018

Contributor

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).

Contributor

dustyhoppe commented Mar 31, 2018

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment