Consider merging sync and async policies / other syntax proposals

[reisenberger]

@juancarrey I'm taking your comment from #140 out into this separate issue:

also implemented a CombinedPolicy which "merges" an async policy with a sync
policy, removing the runtime exceptions of using an async policy synchronously and a sync policy
asynchronously.

Yes, this is well worth a deeper look. The sync/async split was imposed before I was involved with Polly; it can feel odd.

Some considerations:

• We may be stuck with the/some dual MyPolicy / MyPolicyAsync syntax for backwards compatibility.
  • EDIT: The signatures of onBreak and onRetry etc delegates also differ between sync and async policies, so the configuration syntaxes would have to remain separate.
• For people who configure-use-dispose policies all in one statement, or will only use one variant, it would make for an extra (unused) allocation - to consider whether an issue?
  • EDIT: It would mean an extra allocation at configuration time, but one less check (sync/async mismatch check) at execution time. For most users then, in performance terms overall then, net positive.

Not against - it would be great to get rid of this split! 😄 - just thinking through a bunch of angles to this.

Comments from anyone welcome.

Thanks for all the great feedback: keep the suggestions coming!

[brunolauze]

@reisenberger After having played with Polly and looking at the different proposals and our own requirements/desires, I wanted to push the way we envision to use Polly. I reworked all the code so it fits more the goal we are trying to achieve. The branch is located at https://github.com/brunolauze/Polly/tree/polly-reboot.

Almost all the applicable specs passes, and specs like ones in FallbackSpecs are best to show vision.

Here's one typical envisioned policy the refactoring now makes possible:

Policy.Throttle(10)
.ThenTimeout(3000)
.Or<ArgumentException>()
.OrResult<int>((x) => x < 1)
.CircuitBreaker()
.Fallback<int>(() => 1000)

Which reads as follow:

Fallback to value 1000 in a Circuit Breaker fashion when the execution times out of 3 seconds, raises an ArgumentException or the result is less than 1 while ensuring a maximum concurrency level of 10.

Of course it's far from perfect but I think it stir the course in the direction we would like to take internally.

Our main goal here is to add two other component : Polly.Config and Polly.Web
and to have a asp.net (4.5/core) Controller with this kind of action:

[HttpGet]
[Policy("StandardCircuitBreaker")]
public IEnumerable Get()
{

}

and have the ActionInvoker wrapped to Execute within the named policy present in the configuration.

As always, everyone love comments!

[reisenberger]

@brunolauze It's great to see this deep engagement with the Polly codebase - thank you for your time and involvement.

As mentioned off-github, my availability is restricted for a few weeks, 48 hours hence. I can't give a considered response on all the changes within that timespan, so some general/abstract qs and observations only now.

We can't promise also at this stage (obviously) that core Polly will go the same direction on all points. However, it's _great to see so many ideas. Particularly, how to structure the Polly syntax as the number of policies grows has been a key concern, so good to consider alternative syntaxes. @ All: community input on syntax welcome!

...

Setting aside implementations, the main thrust seems an alternative syntax for PolicyWrap - fair observation? Is it fair to characterise as a configure-all-in-one-go (extended-fluent-chain) approach? (a move from static configuration to chaining-on-instances, as briefly floated in #104 ?)

This is a really interesting approach, and one we should definitely consider. Conciseness appears a key benefit, at least for more simple cases.

From another angle, I see this syntax particularly enabling "configure all policychain elements at point of use / in one go" (as distinct from, say, combining preconfigured policychain elements, perhaps passed in from outside). (By elements, I mean the different aspects that might make up a wrap - retry, breaker, etc; by outside I mean, say, passed in from outside the current class).

That seems the key difference to me. (NB At this stage, seeking just to call out the qualities of things - understanding the qualities deeply helps us see where proposals fit and see what range of features to provide!)

Thanks again @brunolauze for the huge contribution in terms of code and ideas - it would be good to see what we can make of this in terms of PRs, at the appropriate moment.

[reisenberger]

@brunolauze Re:

Our main goal here is to add two other component : Polly.Config and Polly.Web

We would be happy to consider PRs on these at an appropriate point. On config, briefly, you may have seen seen some of the discussion to date in issues #143 and #50 ? On Polly.Web, I threw together some samples for the App-vNext team of Polly policies in Asp.Net middleware/pipeline a while back, but didn't have time to turn this into something concrete - so it would be great to see ideas.

[reisenberger]

@brunolauze On the policy wrap/chain briefly: One other aspect to consider is whether policies (eg retry, circuit-breaker) can be re-used in more than one policy chain/wrap. In some cases there are some specific functional advantages to being able to eg re-use the same circuit-breaker across more than one call site : all calls to a common subsystem can share knowledge about the state the subsystem is in.

Something similar could also apply for throttle: one might to share a resource pool across a set of calls in a larger system, rather than (say) define lots of similar smaller resource pools.

[reisenberger]

A key tension/crossroads I see now for Polly is how to continue to grow the power (with syntax allowing powerful options), while still maintaining a simpler syntax for those who want a more off-the-shelf, more-things-preconfigured-perhaps happy path. All community input on syntax welcome, as this a key decision now.

@brunolauze’s above speaks to this problem by offering a way to configure an entire PolicyWrap in a single extended chaining syntax (as opposed to combining previously configured policies).

@RehanSaeed’s #143 speaks to this problem by offering a way to configure a Policy (or multiple policies?/policy alternatives?) out of a single POCO class (while perhaps omitting some of the deeper power like onRetry, onBreak etc delegates – unless this were all piled into the same POCO class - it could be).

@RehanSaeed I see particular possible application for your idea as the new resilience policies and PolicyWrap are added. I haven’t wanted to take Polly in the direction of having a single Command class with settable properties as its underlying operating architecture, as Hystrix has, for the reasons set out in the comparison here. However, your idea of a single POCO configuring class does provide a possible bridge in this direction for the API, by offering a single class where all elements of a PolicyWrap could be configured.

The full PolicyWrap syntax would still offer deeper power, but there will be many users also who just want a pre-defined template for a PolicyWrap that’s suitable for most cases, with a few extra options.

Community feedback welcome

[RehanSaeed]

@reisenberger I see a POCO as a very low level concept that most people would not use. All config providers could be based on it, as well as the other cool features that you have just made me aware of (Fallback's seem like a very good idea and something I'm really interested in e.g. falling back from a cache to the database).

[brunolauze]

@reisenberger we're happy to help because we find it useful for our corporate developement to invest some time in libs like this one. I totally understand your availability please don't worry.

(a move from static configuration to chaining-on-instances, as briefly floated in #104 ?)

The current approch in polly-reboot branch was to have Non Exception Policy into static methods and have those Non Exception Policy have their .ThenXXX like .ThenTimeout() extension methods
Another aspect we liked is the introduction of .ThenHandle<> which is a kind of reset of the stack of Exception handling. We can then have Chain Cascade Type of Exception.
The opposite of .Or<> if you will

.Handle<ProductNotFoundException>()
.Fallback(x => 10)
.ThenHandle<HttpException>()
.Fallback(x => 0)

if my http backend (or sql/whatever) is down i want 0, if my product is just not found (ProductNotFoundException) then 10.

For the config part:

I see something more generic but documented as a start I pushed this https://github.com/brunolauze/Polly.Config
Using System.Configuration with syntax like

<polly>
<policy key="policy1">
<add key="handleException" type="handle" exceptionType="System.Exception" />
<add key="retry" type="retry" retryCount="3"  />
<add key="fallback" type="fallback" value="50" valueType="double" --- valueProviderType="" ---  />
</policy>
</polly>

where Policy.Resolve("poilcy1") would equals:

Policy.Handle<Exception>()
.Retry(3)
.Fallback(() => 50)

And then re-implementing the same but with Microsoft.Extensions.Configuration with a configuration looking like this:

"polly": {
    "retry": {
      "handle": {
        "exceptionType": "System.Exception"
      },
      "retry": {
        "retryCount": 3
      }
    }
  }

I think you should continue looking and tell me if I can do anything. I pushed the changes of timeout policy in my according branch for the current implementation.

[reisenberger]

Closing due to lack of recent activity/comment, and issues now covered elsewhere. To briefly summarise:

Merge sync/async policies: The current position is that this is hard to do; a new, detailed blog post explores the reasons.

Alternative syntax for PolicyWraps: The existing syntax, building a PolicyWrap from granular chunks each being an existing Policy, was preferred as less complex and more flexible over the longer term.

Configuration from POCOs: Remains on the Roadmap as of date of posting.

Config approaches: Linked to here from the main config issue #50