Add support for policy registry #66

rynowak · 2018-02-24T04:18:01Z

Here's a straw man to discuss for the design of retrieving policies from
the registry for IHttpClientFactory.

Goals:

Keep simple usage simple
Support updates (opt in)
Allow use of policies by name and with code
Allow dynamism (choosing policy based on request)

Non-goal:

Avoid overloading based on Func<> types with same arity. This is a
destructive case for intellisense and should be avoided.

rynowak · 2018-02-24T04:19:14Z

samples/HttpClientFactorySample/Program.cs

+                return request.Method == HttpMethod.Get ?
+                    reg.Get<IAsyncPolicy<HttpResponseMessage>>("regular") :
+                    reg.Get<IAsyncPolicy<HttpResponseMessage>>("long");
+            })


This is the best place to start looking. Let's try to nail down what the configuration experience is like and then let the actual implementation take care of itself 😆

rynowak · 2018-02-24T04:22:57Z

src/Microsoft.Extensions.Http.Polly/DependencyInjection/PollyHttpClientBuilderExtensions.cs

+                if (!registry.TryGet<IAsyncPolicy<HttpResponseMessage>>(policyKey, out var policy))
+                {
+                    policy = registry.Get<IAsyncPolicy>(policyKey).WrapAsync(Policy.NoOpAsync<HttpResponseMessage>());
+                }


@reisenberger - there's a lot of tension caused in this design by the desire to support IAsyncPolicy and IAsyncPolicy<HttpResponseMessage>.

For instance if we were to support Func<HttpRequestMessage, IAsyncPolicy<HttpResponseMessage>> and Func<HttpRequestMessage, IAsyncPolicy>, then we end up with a method that's overloaded on the return type of Func<..T>. In general this case gives really bad intellisense and I try to avoid it.

It seems like it's always possible to create a PolicyAsync<T> instead of a PolicyAsync for anything you desire. Thoughts?

a lot of tension caused in this design by the desire to support IAsyncPolicy and IAsyncPolicy<HttpResponseMessage>.

Supporting only IAsyncPolicy<HttpResponseMessage> would certainly simplify the implementation. With HttpClientFactory there is a better case for supporting only generic policies than with Polly in general, given all calls are in HttpResponseMessage.

A concern is that not supporting non-generic IAsyncPolicy will lead to enquiries of why, eg, .AddPolicyHandler(Policy.TimeoutAsync(10)) doesn't compile. In Polly's API, I opted to take the hit and provide bridging overloads (eg in PolicyWrap), so that from the user's perspective, "everything just worked". 'Traffic-managing' resilience strategies like Timeout and Bulkhead have no interest in returned results, so we opted to ensure that non-generic forms could just be used wherever a generic form might otherwise be expected.

If the decision is that HttpClientFactory only supports IAsyncPolicy<HttpResponseMessage>, documentation should cover this, to reduce user blocks, and enquiries.

Polly could add a helper extension method Policy<TResult> Policy.For<TResult>() (internally, a unary-wrapper, a version of .WrapAsync(Policy.NoOpAsync<HttpResponseMessage>()) without the need for the extra NoOpPolicy<TResult>). This could either help users bridge; or HttpClientFactory provide the extra overloads.

Worth further thought / user input / discussion, as will be key to the user experience.

I had a discussion this morning about this with @glennc - we're thinking that we want to try removing/not-providing the non-generic methods for the preview release and see if we get feedback.

There's a relatively small delta between .DoThing(Policy.Timeout(...)) and .DoThing(Policy.Timeout<HttpResponseMessage>()). We'd rather require the extra typing than ship partial or complicated support for non-generic policies.

If we get the feedback that this is important then we will do the work to add overloads that directly accept non-generic IAsyncPolicy.

--

I agree that it would be a nice addition to have a simpler way to go from IAsyncPolicy->IAsyncPolicy<>. I suppose that's on you guys if you think it's worth doing. For now I plan to just document how to do it.

I think this is a balanced approach

We don't really know whether people will miss the non-generic methods so it's probably fine to leave it out and see who complains.

@rynowak Very happy with that call 👍; I agonised over this.

Offering the mix of non-generic and generic overloads makes sense for Polly generally, as we have a range of use cases to support, some of which suit non-generic policies better; but with HttpClientFactory's one use case (for Polly), there is a chance to reduce complexity. Simplicity is a great virtue.

rynowak · 2018-02-24T04:23:48Z

/cc @reisenberger @joelhulen @stevejgordon @martincostello @bryanjhogan

rynowak · 2018-02-24T04:25:07Z

src/Microsoft.Extensions.Http.Polly/DependencyInjection/PollyServiceCollectionExtensions.cs

+   /// Provides convenience extension methods to register <see cref="IPolicyRegistry{String}"/> and 
+   /// <see cref="IReadOnlyPolicyRegistry{String}"/> in the service collection.
+    /// </summary>
+    public static class PollyServiceCollectionExtensions


These are just for convenience. Our AddPolicyHandlerFromRegistry methods will require you to have registered IReadOnlyPolicyRegistry<string> somehow. We don't care if you used these methods to do it.

martincostello

A question about Context and two minor nits, but otherwise this looks nice.

martincostello · 2018-02-24T08:42:47Z

src/Microsoft.Extensions.Http.Polly/DependencyInjection/PollyServiceCollectionExtensions.cs

+    public static class PollyServiceCollectionExtensions
+    {
+        /// <summary>
+        /// Registers and empty <see cref="PolicyRegistry"/> in the service collection with service types


Typo: "Registers an empty."

martincostello · 2018-02-24T08:44:44Z

src/Microsoft.Extensions.Http.Polly/PolicyHttpMessageHandler.cs


-            return _policy.ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);
+            var policy = ResolvePolicy(request);
+            return policy.ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);


Should there be a null-check on policy after ResolvePolicy() returns?

I agonized over this and couldn't really come to a good decision. Usually my philosophy is play stupid games, win stupid prizes...

This can be done in a way that doesn't impact the perf of the non-dynamic way so it's probably worth doing to allay confusion.

martincostello · 2018-02-24T08:46:38Z

src/Microsoft.Extensions.Http.Polly/PolicyHttpMessageHandler.cs


-            return _policy.ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);
+            var policy = ResolvePolicy(request);
+            return policy.ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);


Does there need to be a mechanic here to provide a way to create a Context which could be passed into the policy, or is that going to be catered for separately/elsewhere? Given that the policy is already being resolved from the request, there could be a hook to resolve a context from the request as well.

See #65

Whomever is the caller can provide a context, or another message handler can do it (since they are also callers).

bryanjhogan · 2018-02-24T16:40:14Z

src/Microsoft.Extensions.Http.Polly/PolicyHttpMessageHandler.cs

+            return policy.ExecuteAsync((ct) => base.SendAsync(request, ct), cancellationToken);
+        }
+
+        private IAsyncPolicy<HttpResponseMessage> ResolvePolicy(HttpRequestMessage request)


@rynowak this is great, I think it solves most of the things I was concerned about.

reisenberger

@rynowak This looks to fulfil the goals you set out for retrieving policies from PolicyRegistry for HttpClientFactory:

Support a registry which may dynamically update the configuration of a policy
Allow policy selection (choosing policy based on request)

Two observations:

we could extract 'what-to-handle' from its overload, to make it reusable
the funcs would/should generally select from amongst stable instances (unless config changes), so 'factory' may be misleading?

reisenberger · 2018-02-25T20:45:13Z

src/Microsoft.Extensions.Http.Polly/DependencyInjection/PollyHttpClientBuilderExtensions.cs


+        public static IHttpClientBuilder AddPolicyHandler(
+            this IHttpClientBuilder builder, 
+            Func<HttpRequestMessage, IAsyncPolicy<HttpResponseMessage>> policyFactory)


Suggest: these parameters be called policySelector (or policyProvider?) throughout. policyFactory may suggest users are expected to new up a policy within the Func.

Selecting a previously-config'd policy from somewhere should usually be the pattern. Per previous discussion, with stateful policies like CircuitBreaker, the Func would need to be returning the same instance consistently, in order to maintain circuit state over time. Only if the config is changing should a new instance be supplied.

Sure, that sounds fine.

reisenberger · 2018-02-25T22:30:07Z

src/Microsoft.Extensions.Http.Polly/DependencyInjection/PollyHttpClientBuilderExtensions.cs

        /// </para>
        /// </remarks>
        public static IHttpClientBuilder AddServerErrorPolicyHandler(
            this IHttpClientBuilder builder, 


Wondering if AddServerErrorPolicyHandler(...) is starting to feel disjoint from the others now? It fulfils its essence, to encode errors worth handling, but that 'what-to-handle' power is locked in, not available with its sibling 'how-to-handle' overloads.

Option: Extract a method just returning the PolicyBuilder<HttpResponseMessage> policyBuilder (what-to-handle) it defines, say .HandleTransientHttpErrors().

Users can then use .HandleTransientHttpErrors() anywhere, including for policies provided by the sibling 'how-to-handle' overloads.

Users can riff on it flexibly, eg:

.HandleTransientHttpErrors().Or<TimeoutRejectedException>() - if they were using Polly TimeoutPolicy on inner tries, and wanted a wrapping retry policy to handle those timeouts.

.HandleTransientHttpErrors().OrResult(r => r.StatusCode == HttpStatusCode.RequestTimeout) - or whatever, perhaps some custom status code a particular endpoint returns.

Q then becomes where to site this method, as it's a static method. HttpClientFactory.HandleTransientHttpErrors() ??
Option: we could take it back into Polly, where it would sit in an expected place alongside other Handle syntax:

Policy.HandleTransientHttpErrors()

Thoughts?

Variant: If extracting .HandleTransientHttpErrors(), HttpClientFactory could still retain .AddServerErrorPolicyHandler(...) (using the extracted method), if desired to keep that simple usage story within HttpClientFactory.

I think this feedback makes sense. I'm going to consider this out of scope for this PR thought, will address it in a follow up.

bryanjhogan · 2018-02-26T22:12:23Z

src/Microsoft.Extensions.Http.Polly/PolicyHttpMessageHandler.cs

+
+        private IAsyncPolicy<HttpResponseMessage> ResolvePolicy(HttpRequestMessage request)
+        {
+            return _policy ?? _policyFactory(request);


@rynowak How do you plan to make to PolicyRegistry accessible from this class?

I don't plan to do that. You provide a lambda that resolves the policy, take a look at the sample and let me know if that won't meet your needs.

Here's a straw man to discuss for the design of retrieving policies from the registry for IHttpClientFactory. Goals: - Keep simple usage simple - Support updates (opt in) - Allow use of policies by name and with code - Allow dynamism (choosing policy based on request) Non-goal: - Avoid overloading based on Func<> types with same arity. This is a destructive case for intellisense and should be avoided.

rynowak · 2018-02-28T07:47:05Z

I've addressed all of the minor feedback here. Planning to add tests tomorrow

rynowak · 2018-02-28T07:50:26Z

test/Microsoft.Extensions.Http.Test/DefaultHttpClientFactoryTest.cs

            // Assert - 2
            Assert.Empty(factory._activeHandlers);
-            Assert.True(factory.CleanupTimerStarted.IsSet);
+            Assert.True(factory.CleanupTimerStarted.IsSet, "Cleanup timer started");


This is just misc cleanup that snuck in while I was looking at a flaky test 😆

rynowak · 2018-02-28T19:31:26Z

Updated and added tests. I converted the existing builder tests into end-to-ends.

rynowak commented Feb 24, 2018

View reviewed changes

martincostello reviewed Feb 24, 2018

View reviewed changes

bryanjhogan reviewed Feb 24, 2018

View reviewed changes

reisenberger reviewed Feb 26, 2018

View reviewed changes

bryanjhogan reviewed Feb 26, 2018

View reviewed changes

rynowak added 2 commits February 27, 2018 23:20

rebase and minor feedback

30a6862

rynowak force-pushed the rynowak/polly-registry branch from 33ce079 to 30a6862 Compare February 28, 2018 07:46

rynowak commented Feb 28, 2018

View reviewed changes

rynowak changed the title ~~Design proposal for configuration for registry~~ Add support for policy registry Feb 28, 2018

Add tests

b268058

rynowak force-pushed the rynowak/polly-registry branch from 2804537 to b268058 Compare February 28, 2018 20:29

rynowak merged commit df30496 into dev Mar 3, 2018

rynowak deleted the rynowak/polly-registry branch March 3, 2018 06:13

halter73 mentioned this pull request Oct 15, 2020

Add new Polly DI extension method for HttpClientFactory dotnet/extensions#3142

Closed

Add support for policy registry #66

Add support for policy registry #66

Uh oh!

Conversation

rynowak commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reisenberger Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martincostello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reisenberger left a comment

Choose a reason for hiding this comment

Uh oh!

reisenberger Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rynowak Feb 24, 2018 •

edited

Loading

reisenberger Feb 25, 2018 •

edited

Loading

reisenberger Feb 25, 2018 •

edited

Loading