Add Polly

[rynowak]

Todo like for this:

• Add basic polly handler and builder support
• Add support for polly context
• Add builder methods for integration with the policy registry Start to flesh out Polly design #58 (comment)
• Do we need an opinionated retry-focused extension method Start to flesh out Polly design #58 (comment)

For docs:

• Add documentation about timeouts, see: Start to flesh out Polly design #58 (comment)
• Add documentation about kinds of policies and which builder methods to use Start to flesh out Polly design #58 (comment)

[stevejgordon]

Can I check... is the intention to include some pre-designed DelegatingHandlers utilising Polly for retry, circuit breaker, caching etc "in the box" with the release? Or simply to include some samples? I'm considering starting to develop a policy inside a delegating handler of my own for testing an approach to something we currently do more manually. Trying to decide how much time to focus on that and whether to blog about it or wait until I see any default ones.

[rynowak]

I think it's important that whatever we provide exposes Polly's API in a high fidelity way. They have a lot of different options and ways of doing things.

Goals:

1. Expose their APIs (don't create wrappers)
2. Make it possible to use most of what they provide without having to author a handler
3. Provide a good logging experience for common usage

Any time we interact with a community project we're ask the question, "why should Microsoft build this?" A good reason for Microsoft to build it in this case is that we think having access to Polly for retries and circuit breakers is pretty fundamental. We also really want the logging to great.

---

Right now my (and @glennc)'s thinking is that we'll provide an extension on the builder method that looks like .AddPollyHandler(Func<AsyncPolicyBuilder, AsyncPolicyBuilder>). This is far from decided.

If you compare this to my sample - it means that you would write code like:

.AddPollyHandler(policy.WaitAndRetryAsync(5, (n) => TimeSpan.FromSeconds(n))

and the result would be what I showed in my sample.

There are a few challenges with this, naming figuring out how we get logging into the picture.

[stevejgordon]

@rynowak Makes sense

I'm still figuring out how we might improve our use of Polly locally and thought I'd throw out some thoughts. One thing we're considering here is moving common Polly Policies into a library that we can share between projects. We'd probably have individual Policies and some PolicyWraps defined in there. We're looking at registering these with a PolicyRegistry.

I'm wondering if an extension along the lines of .AddPollyHandlerFromRegistry(string name) would work there? Behind the scenes this could grab the IPolicyRegistry, find the policy and wrap the SendAsync call in it?

For your example above, would it not also be possible to simply have .AddPollyHandler(Policy aPollyPolicyInstance) where we have instances of Policies already available to pass straight in?

[rynowak]

Sure, that's a good suggestion as well.

Whatever we do, the handler will be public and constructable as well so you can do whatever you want this it. So if there's something we don't expose via the builder, it will still be possible to reuse the handler.

[theezak]

The ability to provide conditions for when to use the Polly handler (or any other handler) would be helpful as well. I would like to by pass the Polly handler for any non-idempotent operations (for example POST). It is a dangerous assumption to retry a failed POST because you don't know if the item was actually POST-ed or not.

[reisenberger]

( disclosure: I maintain Polly )

@stevejgordon Yes, at its simplest, a generalised DelegatingHandler taking a single Polly policy could be:

public class ResilientHandler : DelegatingHandler
{
    private IAsyncPolicy<HttpResponseMessage> _resiliencePolicy

    public ResilientHandler(IAsyncPolicy<HttpResponseMessage> policy)
    {
       _resiliencePolicy = policy ?? throw new ArgumentNullException(nameof(policy));
    }

    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        => _resiliencePolicy.ExecuteAsync(ct => base.SendAsync(request, ct), cancellationToken);
}

(And that single Polly policy could of course be a PolicyWrap embodying a combined resilience strategy.)

( @rynowak @glennc I think the above code is close to the sketch floated in earlier discussions, but obviously we are entirely open to how you may want to approach this or build around it.* )

One observation: Making the policy parameter of type IAsyncPolicy<HttpResponseMessage> (as shown above) rather than the Policy base class carries a few possible benefits:

• the generic policies in IAsyncPolicy<HttpResponseMessage> are marginally more performant executing delegates returning HttpResponseMessage, than their non-generic counterparts IAsyncPolicy or Policy
• the policies with generic type parameter HttpResponseMessage allow users to configure policies which can handle (respond to) any content of the HttpResponseMessage, in addition to exceptions thrown. For example, policies could handle result codes directly as described here.
• it will limit the supplied policy to policies which can execute async delegates returning HttpResponseMessage (the only case for a DelegatingHandler anyway).

---

EDIT: *Noted also the comments in the ASP.NET Core 2.1 roadmap

[reisenberger]

Responding to @stevejgordon and @rynowak re possible HttpClientFactory-PolicyRegistry integration:

I like the approach here as syntactic shortening for one-time configuring policies on HttpClientFactory from PolicyRegistry.

A consideration is that we (Polly) are starting to recommend PolicyRegistry as a route to dynamic reconfiguration of policies during running. This could imply a slightly different approach such as:

// [a]
public static IHttpClientBuilder AddPolicyHandler<T>(this IHttpClientBuilder builder, string namedPolicy) where T : IPolicyRegistry<string>
{
	IReadOnlyPolicyRegistry<string> registry;

	using (var sp = builder.Services.BuildServiceProvider())
	{
		registry = sp.GetService<T>();
	}
	
	if (registry != null)
	{
		builder.AddHttpMessageHandler(() => new PolicyHttpMessageHandler(registry.Get<IAsyncPolicy<HttpResponseMessage>>(namedPolicy)));
	}

	return builder;
}

The intention is that PolicyRegistry can become a dynamic store (updated by a separate code stream when the underlying configuration source changes), and (in this case) as HttpClients/DelegatingHandlers are recycled, the policy picked out of the registry by the configuration func might match previous or be an updated one.

I am not close enough @rynowak to the expected dynamics of HttpClient/DelegatingHandler recycling in HttpClientFactory (how often would they be recycled?; how long might we expect 'old' and 'newer' instances to co-exist?), to know whether this is a story that would play well with HttpClientFactory. And dynamic reconfiguration of Polly policies is still a developing story. So I'm not necessarily recommending right now that HttpClientFactory take the above approach [a]. However, it might be an argument for not embedding static-configuration as the main overload for PolicyRegistry in HttpClientFactory at this point.

Alternatively, configuration overloads could (if demand) divide into AddPolicyHandler(...) and AddDynamicPolicyHandler(...), without that necessarily being aligned to not-via-registry and via-registry. But much depends on whether dynamic reconfiguration plays well with client/handler recycling at all. The idea also appears to go against my previous comment on Policy state wanting to remain stable across recycling; but here, the change of state would at least be intentionally triggered, so may be acceptable, if overlap between old and new policies were short-lived.

Sharing perhaps as much for future consideration as for this version; but in case influences any decisions now.

All thoughts welcome. (Thanks @stevejgordon for such great input!)

[martincostello]

For what it's worth, I've recently plugged Polly into HTTP calls to dependent services in an API I've been working on, and we do something similar to what's mentioned above with regards to dynamic updates to policies.

At a high-level, it works something like this:

• Singleton custom policy store object registered in DI that wraps PolicyRegistry and makes Policy objects on-demand for specific API hostnames. These carry other configuration with them like timeout, bulkhead limits, circuit breaker break periods etc. as a strongly-typed object bound from IConfiguration. These policies are created and added if not already in the registry for a specific hostname; subsequent calls into this abstraction get the same appropriate policy for the lifetime of the application so state is maintained.
• HttpClientHandler registered as a singleton. This is configured with other message handler we use, such as for monitoring and adding custom request headers common across our APIs.
• HttpClient registered as a transient dependency that uses the singleton HttpClientHandler. This allows us to use properties like Timeout and BaseAddress freely.
• Refit is used to register transient strongly-typed clients, which use HttpClients resolved from DI.
• Other transient abstractions use policy store and the strongly-typed Refit-based interfaces to make HTTP requests as required by the application through policies obtained for the endpoint from the policy store.

We also use Consul as a configuration store, which allows us to make changes to the application configuration at runtime. When configuration changes are made in Consul, we perform an HTTP POST to a custom endpoint in the application, which resolves the singleton policy store from DI and instructs it to clear the PolicyRegistry. This then causes the Policy objects to be recreated on the next request for them from the API abstractions.

While this setup differs from HttpClientFactory because Polly is specifically invoked, rather than abstracted away as a message handler, I thought I'd include the detail of our setup here as an example of a real-world production use-case.

We're actively following the progress in this repository, and are likely to try and adopt this in our new .NET Core applications once ASP.NET Core 2.1 ships.

The less we need to customise and the more that "just works" from using HttpClientFactory the better 😃

[reisenberger]

@martincostello Awesome to hear this. It sounds like you have PolicyRegistry-as-dynamic-source, with events triggering update, elements of policy-configuration-from-strongly-typed-POCOs, and Policy-objects-scoped-per-API-hostname, which sounds like the scoping strategy recently discussed for HttpClientFactory.

@rynowak My previous comment was tentative about pursuing dynamic-policy-reconfiguration, pending deeper input on HttpClient/HttpMessageHandler recycling. However, we could bypass policy updates hanging on HttpClient/HttpMessageHandler recycling, by pushing the func extracting policy from registry into the HttpMessageHandler:

    public class DynamicPolicyHttpMessageHandler : DelegatingHandler
    {
        private readonly Func<IAsyncPolicy<HttpResponseMessage>> _policyFunc;

        /// <summary>
        /// Creates a new <see cref="DynamicPolicyHttpMessageHandler"/>.
        /// </summary>
        /// <param name="policyFunc">A function returning the policy to use.</param>
        public DynamicPolicyHttpMessageHandler(Func<IAsyncPolicy<HttpResponseMessage>> policyFunc)
        {
            if (policyFunc == null)
            {
                throw new ArgumentNullException(nameof(policyFunc));
            }

            _policyFunc = policyFunc;
        }
        
        /// <inheritdoc />
        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            if (request == null)
            {
                throw new ArgumentNullException(nameof(request));
            }

            return _policyFunc().ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);
        }
    }

This doesn't hard-code any relation to PolicyRegistry, so is generalised - usable without any assumption a dynamic PolicyRegistry underpins. For the dynamic PolicyRegistry case, however, @stevejgordon 's original configuration overload could change to / or be supplemented with:

// [b]
public static IHttpClientBuilder AddDynamicPolicyHandler<T>(this IHttpClientBuilder builder, string namedPolicy) where T : IPolicyRegistry<string>
{
	IReadOnlyPolicyRegistry<string> registry;

	using (var sp = builder.Services.BuildServiceProvider())
	{
		registry = sp.GetService<T>();
	}
	
	if (registry != null) // or stronger null-guard
	{
		builder.AddHttpMessageHandler(() => new DynamicPolicyHttpMessageHandler(() => registry.Get<IAsyncPolicy<HttpResponseMessage>>(namedPolicy)));
	}

	return builder;
}

This makes updates to policies 'instant', at the expense of an extra dictionary lookup (within PolicyRegistry) per call. Time-wise, new and old configurations of a policy could overlap only for the extent that executions were still in flight through the outgoing policy instance while some executions had started using the new version of same-named policy.

---

There could also be ways to avoid the expense of the dictionary-lookup-per-call, if desired to optimise it away. For instance, Polly's default PolicyRegistry implementation could raise OnPolicyChanged events for which consumers like DynamicPolicyHttpMessageHandler could register a handler, to update their copy of the policy, on changes.

---

@rynowak @martincostello @stevejgordon I am all for getting dynamic policy reconfiguration into this version of HttpClientFactory, if we have an approach we jointly like and wish to pursue. Having floated a number of ideas, probably time I step back and let the MS folks speak to this now.

[joelhulen]

+1 on the OnPolicyChanged events. I love the idea of instantly updating policies. I know we've talked about this before, having dynamic reconfiguration at some point. PolicyRegistry did a lot to bring us closer to that reality. I think that the conversation we're having here definitely shows how locking down the best way to do this is becoming a higher priority. @martincostello, great write-up on how you're approaching some of these challenges in a real world environment. It's exciting to see the momentum building!

[bryanjhogan]

Looking at the sample suggests that we can only have a single policy for a given HttpClient. Am I right about this?

I ask because I may want to use a different policy for GET, PUT, POST, etc. for a given endpoint, and I will use the same httpClient for for all requests to the endpoint.

Is there a reason why we would not want a syntax where we pass the policy to the HttpClient.Get/Post methods at the point of execution? Something like -

_httpClient.GetAsync(requestEndpoint, _waitAndRetryPolicy)

I know this has the drawback that now you need to have the policy available in all places you want to execute a request, but the policy registry solves that. And of course the code of HttpClient needs to change or be extended, which might be a bigger issue.

[martincostello]

@bryanjhogan maybe the policy depending on the HTTP verb could be enabled for the DynamicPolicyHttpMessageHandler example from @reisenberger above by changing the Func<> to have a single parameter of HttpRequestMessage?

[bryanjhogan]

Thanks @martincostello (btw, have used httpclient-interception, very useful).

Now that I think about it, I have plenty of scenarios where I want to use different polices for GET requests to a remote host, One request uses the cache policy, another the timeout policy, another the wait and retry - all request are made to the same remote host.

Based on the code example #60

var myPolicy = Policy....; // create a policy once

services.AddHttpClient("github").AddPolicy(myPolicy);
services.AddHttpClient("facebook").AddPolicy(myPolicy);

It looks like all requests to github will use the same single policy and all requests to facebook will use the same single policy.

[rynowak]

It looks like all requests to github will use the same single policy and all requests to facebook will use the same single policy.

That's been the plan so far. There's no reason why wouldn't make something like that easy though. I think I have some good ideas based on what's been discussed here so far. I will start drafting something and loop you all in for feedback/examples.

[rynowak]

Responding directly to a few other comments that have been made here.....

From @reisenberger

I am not close enough @rynowak to the expected dynamics of HttpClient/DelegatingHandler recycling in HttpClientFactory (how often would they be recycled?; how long might we expect 'old' and 'newer' instances to co-exist?)

Handler rotation moves an 'active' handler to the 'retirement' state on a fixed interval. There is at most one 'active' handler per-name at a time -- however the 'retired' handler is still valid and usable until it goes out of scope.

We accomplish this by using weakreferences and a 'dummy' handler - letting the GC tell us one when of the 'dummy' handlers is no longer reachable. Then we know to disposed and clean up the 'real' handler pipeline. This works well since HttpClient doesn't allow you to access the underlying handler chain.

The interval is 2 mins by default. Our guidance is to align the handler rotation with roughly how often you'd like to see DNS update. The purpose of this is to allow the underlying connections to drop and be recreated. Since the handlers are shared while active, the connection cost is amortize.

There's an inherent tradeoff here because reconnecting too often is expensive, but reconnecting never results in stale DNS. I think we ignore any interval less than 5 seconds IIRC.

Long term we expect the managed handler being worked on in dotnet/corefx to solve this problem by doing the same thing internally. But that won't be available everywhere immediately, and it takes time for people to upgrade.

We get a lot of questions about the lifetime and DNS questions because these have historically been problems for most server applications on .NET, and especially if you're not on .NET framework on Windows there is no current good solution. We see this as just one of a few valuable things we're providing. Ultimately solving this problem is best done at the dotnet/corefx level where the details aren't user-visible.

---

From @martincostello

We're actively following the progress in this repository, and are likely to try and adopt this in our new .NET Core applications once ASP.NET Core 2.1 ships.

Glad to hear. Let us know if you feel like something we add doesn't meet your needs or expectations.

We don't aim to solve every possible concern or scenario, but we want the things we are building to be flexible and broadly useful.

---

@reisenberger @joelhulen

For instance, Polly's default PolicyRegistry implementation could raise OnPolicyChanged events for which consumers like DynamicPolicyHttpMessageHandler could register a handler, to update their copy of the policy, on changes.

If you end up doing something like this, consider exposing IObservable. This is a contract that exists everywhere for a push-based sequence of values. Passing in IObservable<Policy> to a handler is pretty flexible.

[reisenberger]

@rynowak Thanks for the great explanation. This is what I expected from reading the code and from the project background, but it's great to have confirmed, incl. the take on expected usage.

The handler retirement story matches the one we have for dynamically updating policies from PolicyRegistry in other contexts - outgoing instances remain in use just for the lifetime of continuing executions through them - that confirms retiring them together is at least a workable option.

[rynowak]

Closing this as done to capture the progress. The remaining work here is to add logging, for the new integration, which won't be possible in preview2.

Thanks everyone for your feedback. Everything we're doing is substantially better because of your contributions!

Special thanks to @reisenberger @joelhulen for help with Polly and @stevejgordon for working on our docs.