This test is failing on travis occaisionally https://travis-ci.org/aspnet/HttpClientFactory/jobs/344818297

Logging in HttpClients

I want to propose that logging in the HttpClient pipeline is fundamentally two-stage. We log at the beginning of the pipeline (outer) and at the end of the pipeline (inner).

To understand why this is important imagine that you've got message handlers plugged in for reliability (retries) and for service discovery. This might look like:

HttpClient
-> Logging (outer)
-> -> Reliability 
-> -> -> Service Discovery
-> -> -> -> Logging (inner)
-> -> -> -> -> HttpClientHandler (actually does HTTP)

What a request flow might look like (basically) when we have a problem and have to retry - let's say a server goes down - and when we retry the service discovery handler gives us a different server. The logs might look like this (use your imagination):

INFO Starting processing HTTP GET http://myserver/api/products/17
INFO Resolved myserver to myserver1.azure.com
INFO Sending HTTP request GET http://myserver1.azure.com/api/products/17
INFO Got HTTP response 500 - Internal Server Error
INFO Retrying request in 2 sec - retry count 1
INFO Resolved myserver to myserver2.azure.com
INFO Sending HTTP request GET http://myserver2.azure.com/api/products/17
INFO Got HTTP response 200- OK from GET http://myserver2.azure.com/api/products/17
INFO Completed processing 200- OK

Some principles

• Assume the URL will change between the beginning of the pipeline and end
• Assume that there is a 1:* relationship between logical requests (call to HttpClient) and physical requests (actual HTTP)
• Assume individual components will need their own logging (retries, auth, service discovery)
• Assume individual components might need to do their own network calls (auth, service discovery)

Additionally, we want the configuration experience for logging to make you happy. To me that means that you can:

• Adjust logging levels for an individual named HttpClient
• Get enough information from high-verbosity logging to understand what was sent and received in terms of HTTP without needing to debug

Proposal

Ok, now time for a strawman proposal. For the purposes of examples, I'll assume we're using a 'github' named client.

Part 0 Logger Names:

Each client has a named source for the 'outside' (logical) and 'inside' (physical). Here's my humble suggestions:

Outer: `System.Net.Http.HttpClient.github`
Inner: `System.Net.Http.HttpClient.github.ClientHandler`

Additional handlers like service discovery and reliability are encouraged to use source names like System.Net.Http.HttpClient.github.Polly or System.Net.Http.HttpClient.github.ConsulServiceDiscovery

This way you can configure the levels for sources like System.Net.Http.HttpClient and broadly configure all concerns for all clients or System.Net.Http.HttpClient.github and configure all concerns for github.

Part 1 Scopes:

The outer logger will create a scope and attach the initial request HTTP method, and URL.

There's no need for a scope in the inner logger since we'd only be surrounding the HttpClientHandler at that point, and we don't expect the HttpClientHandler to know about our logging framework - these implementations come from the BCL.

I think for now we'd recommend that individual message handlers inside the pipeline only create a scope if they are doing an additional remote call. For instance if I write a message handler that can authenticate and get a token for an API server - then the remote call to get the token should be wrapped in a scope. The default guidance is not to create a scope, but there may be reasons why you want to if the process is complex or involves remote resources.

Part 3 Logging levels:

I think we should treat all 500s as error and log them as errors when they reach the outer handler.

Other than that (concerning 400s) - for now I think we should just use the Info and Debug levels for logging. Trying to decide what's an error or warning is a bit tricky and quickly leads down the path of special-casing specific status codes. I'd rather be cautious here and avoid being too noisy.

Note: Let's get feedback on this, I've thought about it a fair bit and haven't come up with a set of ideas that I really believe in.

Part 4 Default logging at Info level:

The goal of logging at Info level is to present the basic information and help you know whether the request was a success or not. Info level logging could help you know things like "The user's request failed because call to api.github.com is hitting a rate limit". Besides obvious stuff we don't expect info to tell you enough to know exactly what was sent and what was received.

So what this looks like:

Outer: logs outgoing HTTP Method and URL - logs incoming status code and status code text - logs timing
Inner: logs outgoing HTTP Method and URL - logs incoming status code and status code text - logs timing

Note: Is this too verbose? Should we have the inner handler do Debug only? We could also do something like only use the inner handler when you have some other handlers.

Part 5 Default logging at Debug level:

Debug logging - bring on the spam 😆

The goal here is now to give you lots of information about what's going in and out over the wire. Both outer and inner loggers will also log all headers, and the HTTP version, giving you everything except the body.

We also probably want to give you some additional details like what kind of HttpContent you are sending/receiving (when appropriate).

This is nice because you get a few clickstops for configuring logging:

set System.Net.Http.HttpClient.github to Info

Now you get basic high level logging, this is the default experience

set System.Net.Http.HttpClient.github to Info
set System.Net.Http.HttpClient.github.ClientHandler to Debug

Now you get high level logging for each logical operation, but fill input and output logging for each HTTP request that goes over the wire

set System.Net.Http.HttpClient.github to Debug

Now you get the full firehose, including all logging from components like service discovery and polly 👍

Note: What about the body? We could log incoming and outgoing bodies in some selective cases. HttpClient already has support for on-demand buffering. We could consider this on debug, but I kinda want to wait and see.

We need to handle the fact that keeping a HttpClientHandler alive forever results in stale DNS.

Today our strategy is to rotate the handlers periodically.

Is the space character a valid name?

How are you able to call the SendAsync with the same request twice? When you call SendAsync it calls CheckRequestMessage which then calls MarkAsSent which prevents a request object from being used twice.

https://github.com/dotnet/corefx/blob/4b1cf8d60e2ea1d1feac972dfb8e6884fcb45f2f/src/System.Net.Http/src/System/Net/Http/HttpClient.cs#L630

A question on how to use the IHttpClientFactory. I would like to use a named client, but with a HttpClientHandler.

This how I used to create a HttpClient

var handler = new HttpClientHandler
{
    AllowAutoRedirect = false,
    AutomaticDecompression = DecompressionMethods.Deflate | DecompressionMethods.GZip
...
};
_httpClient = new HttpClient(handler);

But I would like to use the IHttpClientFactory like this:

startup.cs

services.AddHttpClient("named", c =>
{
    c.BaseAddress = new Uri("TODO");
    c.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
    c.DefaultRequestHeaders.CacheControl = new CacheControlHeaderValue
    {
        NoCache = true,
        NoStore = true,
        MaxAge = new TimeSpan(0),
        MustRevalidate = true
    };
});

How can I pass the HttpClientHandler to the named HttpClient. I was not able to find this. Any hints on how to achieve this?

Hi guys,

@rynowak cool demo at the community standup.

I was wondering if there is chance to see a an example for Autorest?

Because I often end up in introducing a clientfactory for my autorest clients especially with OAuth.

   public class ApiClientFactory : IApiClientFactory
    {
        private readonly string _authenticationServiceBaseUrl;
        private readonly string _apiBaseUrl;

        public ApiClientFactory (string authenticationServiceBaseUrl, string apiBaseUrl)
        {
            _authenticationServiceBaseUrl = authenticationServiceBaseUrl;
            _apiBaseUrl = apiBaseUrl;
        }

        public IPortalApiClient GetApiClient(AuthenticationCredentials credentials)
        {
            // credentials contain client id, secret, scopes requested ...
            var accesstokenProvider = new AccessTokenProvider(_authenticationServiceBaseUrl,credentials);
            return new ApiClient(new Uri(_apiBaseUrl),new TokenCredentials(accesstokenProvider));
        }
    }

Still not working, despite many broken promises

With this being all in one package that ties the HttpClient caching, the DI extension methods and targeting netcorestandard2.0 it makes it hard for class libraries to pull in just the IHttpClientFactory support.

For example the AWS SDK for .NET which targets .NET Standard 1.3 would not be able to use this library. That is unfortunate because dealing with the HttpClient lifecycle issues is one of our biggest challenges. We don't want to add the other Microsoft.Extension packages dependencies to our SDK though.

Would it be possible to move the code for IHttpClientFactory and its implementation into a separate nuget package targeting .NET Standard 1.3 or less and move the DI extension methods into a separate package? That way IHttpClientFactory becomes useful for things besides ASP.NET Core applications.

Starting and issue to track items we will need to cover in does just so I can add notes and close out other issues.

Using IHttpClientFactory outside of ASP.NET Core

We recommend using the DI container in your other app, even if it's just to bootstrap the factory.

Using IHttpClientFactory + Named Clients in tests

We recommend mocking the IHttpClientClientFactory rather than trying to configure it using DI. The contract is really simple to mock and usually requires new-ing up an HttpClient.

For the HttpClient itself, there are some good libraries already out there that support creating an HttpMessageHandler that will do what you want for the test. See #28 (comment) for some additional discussion. We should document some use cases to these libraries and point to them in our docs.

We want to try and avoid requiring DI in tests - that requires you to open pandora's box.

Hi Guys,

I'm trying to figure out how to add a HttpClientHandler to the http client for authentication purposes. So previously I would do this ...

var handler = new HttpClientHandler
            {
                Credentials = CredentialCache.DefaultNetworkCredentials
            };

            var client = new HttpClient(handler)
            {
                BaseAddress = new Uri(_baseAddress),
            };

client.DefaultRequestHeaders.Add("Prefer", "odata.include-annotations=\"OData.Community.Display.V1.FormattedValue\"");
            client.DefaultRequestHeaders.Add("Prefer", "return=representation");
            client.DefaultRequestHeaders.Add("Accept", "application/json");

I can't figure out how to do this with the HttpClientFactory. Is there any way of doing this currently?

services.AddHttpClient<ICrmHttpClient, CrmHttpClient>(client =>
            {
                client.BaseAddress = new Uri(Configuration.GetValue<string>("CrmWebApiUrl"));
                
                client.DefaultRequestHeaders.Add("Prefer", "odata.include-annotations=\"OData.Community.Display.V1.FormattedValue\"");
                client.DefaultRequestHeaders.Add("Prefer", "return=representation");
                client.DefaultRequestHeaders.Add("Accept", "application/json");

            });

I was just watching the community standup (great demo BTW), and noticed one thing with the log messages - they don't include the HttpClient name in the source.

e.g. For the github named client, the log sources are System.Net.Http.github and System.Net.Http.github.ClientHandler.

That makes it a pain if I want to set a filter for the log messages from all clients. (e.g. to > Warning). I would either need to setup log filtering for each individually named client, or blanket filtering across the whole System.Net.Http namespace.

If you change these lines to be

var outerLogger = _loggerFactory.CreateLogger($"System.Net.Http.HttpClient.{builder.Name}");
var innerLogger = _loggerFactory.CreateLogger($"System.Net.Http.HttpClient.{builder.Name}.ClientHandler");

(or similar), then we can filter on System.Net.Http.HttpClient and it will apply to all clients.

Overhead of creating a client with the factory
Overhead of logging on outgoing
Overhead of Poly policies

This is a question rather than an issue. I have asked this in several places but not been able to get a reliable answer.

My usage of HttpClient is to test the health of HTTP endpoints. As such, I am making calls out to many host/port combinations. The set of endpoints I check should be considered "random". My understanding regarding the reuse of HttpClientHandler is that it is only of benefit when connecting to the same host/port so that the socket can be reused.

My questions are:

• Should I use any caching of HttpClient (HttpClientHandler) instances when connecting to an unknown/random set of host/port combinations?
• How does the API of this factory abstract that away so it doesn't matter to the developer?

If the host/port combination is the point of uniqueness to HttpClientHandler, then shouldn't this factory use the base uri of the endpoint (that represents host/port) in the signature of IHttpClientFactory.Create instead of a name. As an example, I'm guessing that the following pseudocode (while valid against the compiler) is a completely incorrect usage:

var client = factory.Create("test");

client.Get("https://www.firsthost.com/");
client.Get("https://www.firsthost.com:4532/");
client.Get("https://www.someotherhost.com/");

If the purpose of the name parameter is to control a cache around the socket to the host/port, this api does not appear to do that.

I'd love to hear some feedback on this. I'm really keen to know if I'm correct in my thinking or whether there is something I need to understand better.

Cheers

See #24 (comment)

As part of preliminary investigation of how to update the documentation and samples for JustEat.HttpClientInterception for ASP.NET Core 2.1 and HttpClientFactory (see justeattakeaway/httpclient-interception#23), I found that there was a lack of overloads on some extension method types that provided a IServiceProvider for some scenarios.

While not the end of the world in this particular example, I think this might be more of an issue in an application where settings from strongly-typed configuration (i.e. Options<T> things) are wanted by the consuming app to augment the HTTP client builder.

Is there merit in adding overloads similar to the following to allow for such usage patterns?

// HttpClientBuilderExtensions
public static IHttpClientBuilder ConfigureHttpClient(this IHttpClientBuilder builder, Action<HttpClient, IServiceProvider> configureClient);

// HttpClientFactoryServiceCollectionExtensions
public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, string name, Action<HttpClient, IServiceProvider> configureClient);

This question might be more of a question of discoverability, rather than missing functionality, and I'm just looking at things wrong. For example, I later found an overload to AddTypedClient() I missed initially, which lead to another refactor (commit).

At first I was like: 😞 😢 😭 😠

And then I was like: 😕 🤔 😃 👍

https://msdn.microsoft.com/en-us/library/system.net.http.httpclientfactory(v=vs.118).aspx

The fqn System.Net.Http.HttpClientFactory already exists in the old WebAPI client library. This is where we we also provide things like ReadAsAsync<T> methods and the general support for formatters and deserialization. We own this library and control it's destiny 👍

Basically we expect users to use this package and the name HttpClientFactory will conflict between Microsoft.Extensions.Http.HttpClientFactory and and System.Net.Http.HttpClientFactory. This will be broken for a lot of people.

🇨🇦 Fortunately, we Canadians have the same word for crisis as we do for opportunity 🇨🇦.

Please note that most of the types we need to interact with for HttpClient are in System.Net.Http. We were going to add Microsoft.Extensions.Http as a companion namespace. We expected that users will have to sprinkle some Microsoft.Extensions.Http around their code - but this was a non-goal.

Please also note that it is not a breaking change to change a type from static to abstract, in fact a static class is declared as sealed abstract at the IL level - and unsealing is not a breaking change.

At this point a plan starts forming in my mind. Don't reply until you get to the end. I know some of you are going to type a response before you read all of the steps. Here's what I think this would look like:

1. We create a new package in the WSR repo System.Net.Http.XYZ - pick whatever name you want
2. We type-forward HttpClientFactory into this new package
3. System.Net.Http.Formatting now depends on this new package
4. We unseal System.Net.Http.HttpClientFactory and add whatever we decide is required.
5. We ship 5.2.4 of WSR including the new package (already planned)
6. We ship Microsoft.Extensions.Http as the implementation of HttpClientFactory including the wire-ups for DI, logging, and options

Pros

• This is our change to re-use and extend something that already exists
• The Microsoft.Extensions.Http namespace isn't required unless you are implementing extensibility
• Imagine we don't do this, and just choose another name (HttpClientProvider 🤒) this is still going to be annoying for anyone using System.Net.Http.Formatting.

Cons

• Requires another package to exist, or introduces coupling we don't want (see below)
• Some risk associated with getting step 4 nailed down
• Have to live with the existing API on HttpClientFactory which is IMO not that bad

Why another package
This solves an issue with coupling.

Right now HttpClientFactory lives in the Microsoft.AspNet.WebApi.Client package, where it also has a whole bunch of infrastructure for formatters, content negotiation, and also a reference to JSON.NET

We anticipate that Microsoft.Extensions.Http to have a bunch of references to other Microsoft.Extensions.* packages. We really don't want to introduce coupling either way. You don't want things like DI and options showing up in your old ASP.NET WebAPI 2 project, and you don't necessary want WebAPI2 formatters in your ASP.NET Core project.

If we have to live with the coupling one way we'd probably want to reference Microsoft.AspNet.WebApi.Client from Microsoft.Extensions.Http. Most of what's there is still pretty relevant, and it's not coupled to any server-side abstractions.

ActivatorUtilities.CreateInstance is 30x slower than invoking factory delegate.

The tricky thing here is that we think named clients are the primary experience - and that doesn't directly work with DI because we don't have a way for you to provide a name when the container does injection.

We're considering experiences like the following:

Layer 0: Just using the factory (duh)

public class MyComponent
{
    public MyComponent(HttpClientFactory factory)
    {
        Client = factory.CreateClient("github");
    }

    public HttpClient Client { get; set; }
}

services.AddHttpClient("github", builder =>
{
    ...
});

Layer 1: Registering a handle type

Behind the scenes we'll register a factory function for your MyComponent class

public class MyComponent
{
    public MyComponent(HttpClient client)
    {
        Client = client;
    }

    public HttpClient Client { get; set; }
}

services.AddHttpClient<MyComponent>(builder =>
{
    ...
});

Layer 2: Using MVC

MVC's model binder will map a client to a name for you - doesn't work with DI.

public class MyController
{
    public HttpClient GithubClient { get; set; }

    public IActionResult DoThing()
    {
    }
}

services.AddHttpClient("github", builder =>
{
    ...
});

Hi,

I stumbled on this while looking for a HttpClient abstraction. IHttpClientFactory looks pretty good, but how can I grab this as a package to use this in my project? I can only find it here but it would be great to be able to use this in my AspNetCore 2.0.1 project. If it's not available individually, is it part of some other package that is available for consumption?

Nick

Polly v5.9.0 is published. PR will follow to update HttpClientFactory to reference Polly v5.9.0.

TL;DR Polly v5.9.0 includes some deprecations of APIs we will rationalise/rename in Polly v6.0. None of these changes affect HttpClientFactory at 2.1, but adopting Polly v5.9.0 in HttpClientFactory now, before ASP.NET Core 2.1 RTM, avoids any delta in the wider Polly experience for HttpClientFactory users between ASP.NET Core 2.1 and v-next.

Per semver and ASP.NET Engineering Guidelines, Polly v5.9.0 adds the replacement APIs (where relevant) and uses [Obsolete("with an explanation here of the alternative API to be used, where relevant")] to deprecate the APIs which will be removed at Polly v6.0.

Principal changes with relevance to HttpClientFactory

Polly v5.8.0 -> v5.9.0:

[1] rationalises Execute/Async(,...) overloads by deprecating legacy/little-used and anomalous overloads.

• No impact for HttpClientFactory or consumers of HttpClientFactory as PolicyHttpMessageHandler abstracts away the policy.ExecuteAsync(...) call in any case.

[2] clarifies naming of Polly telemetry properties in preparation for richer telemetry/logging from Polly executions.

• No impact for HttpClientFactory as consumption of these properties for logging is not part of ASP.NET Core 2.1 release.

[3] improves syntax for the edge case where users configure a non-generic Policy such as Policy.TimeoutAsync(10); but wish to use it with HttpClientFactory; for HttpClientFactory the decision was to accept generic IAsyncPolicy<HttpResponseMessage> only.

• only affects explanatory XML remarks within HttpClientFactory; change will be PR'd shortly.

Should further detail be required, every change in Polly v5.9.0 is milestone-tagged here and here.

I know it is late for such thing, but it is probably the last chance to introduce IHttpClient interface.
If you introduce factory in 2.1 without interface it will always return a class. An interface is better for testing, mocking, etc. I'm sure that I don't have to prove this 🤓

I can implement this if you only agree 😄

Currently our loggers in debug mode log each header as its own log statement. We have a feeling this may not be ideal from the POV of tools that programmatically consume structured logging. We need to look at some other approaches.

This is an obvious seam that people will use to mock out http clients, we should think about that.

Revisiting this conversation (click also "Show outdated") about cleaning up Polly execution Context. I don't think we necessarily have to change anything - the below doesn't affect any recommended HttpClientFactory usage pattern afaics - but I'm airing an option so we've had the discussion.

Where we currently do this, we could:

var cleanUpContext = false;
var context = request.GetPolicyExecutionContext();
if (context == null)
{
    context = new Context();
    request.SetPolicyExecutionContext(context);
    cleanUpContext = true;
}

var policy = _policy ?? SelectPolicy(request);
HttpResponseMessage response = await policy.ExecuteAsync((c, ct) => SendCoreAsync(request, c, ct), context, cancellationToken);

if (cleanUpContext)
{
    request.SetPolicyExecutionContext(null);
}

return response;

We cannot clean up Context on exiting every PolicyHttpMessageHandler. We need to flow Context inwards and outwards through nested PolicyHttpMessageHandlers so that telemetry for an entire execution (including retries) correlates with the unique CorrelationId on Context.

But if we created the Context, we know both that it was not user-supplied, and that we are the outermost PolicyHttpMessageHandler, so we could clean it up.

Philosophically this feels slightly cleaner, in adhering to the principle of least side-effect: whether the overall call thru HttpClient was supplied an HttpRequestMessage with or without context, we return it in the same state. It's possible this extra cleanliness could prevent us being bitten later in some as-yet-unforeseen case.

Practically-speaking, as you hinted @rynowak, it feels like only some weird edge-cases could be affected. An example could be where a user was re-using an HttpRequestMessage by deep cloning. Cloning has been a solution to certain Use Cases in the past (eg 1; 2). Tho some of those Use Cases will have been around orchestrating retries, for which of course we'll be recommending the PolicyHttpMessageHandler approach. Maybe others can see other cases.

If you do want the extra cleanliness, I have a commit here which we could PR in.

EDIT: Updated (and squashed) the commit for consistency of using var: f332d50

Feedback from experience review is that our builder methods are too verbose and repetitive. We need to do a pass on these and make sure they are good.

Also we want to add a ConfigurePrimaryHandler extension method on the builder to set the client handler easily. We think there are enough use cases that require this (testing, setting options on the handler itself, using a different non-default handler type).

Moving from aspnet/Diagnostics#384

Having just read through this (long) issue relating to HttpClient and DNS TTLs, there are comments made about pooling of clients/handlers so that over time cached IP addresses from DNS entries are evicted. If if understood correctly, pooling is something that's been at least mentioned in this repo, but I'm guessing probably from the socket-exhaustion point-of-view.

Is period flushing of shared instances from a DNS point-of-view something that's being explicitly considered as a use case for things that HTTP client factory would be able to do to help consumers fall into the pit of success for? If not, is it worth incorporating into the design?

var serviceCollection = new ServiceCollection();
serviceCollection.AddLogging(b =>
{
b.AddFilter((category, level) => true); // Spam the world with logs.

            // Add console logger so we can see all the logging produced by the client by default.
            b.AddConsole(c => c.IncludeScopes = true);
        });

We are logging header information when using the Debug LogLevel at the moment. It was pointed out to me that we normally use Trace for anything that could have sensitive information in them and that we probably should do that for headers.

This is a tracking issue and description of the initial HTTPClientFactory implementation.

I will update this in the near future with the description of the overall direction for 2.1.

Issues will be tracked in this project: https://github.com/aspnet/HttpClientFactory/projects/1

In general the 2.1 milestones will be accurate for details of work being done.

Not so much an issue as drawing out the below implication, and noting it for consideration for documentation

Scope of Polly polices in relation to named HttpClient configurations

Ensuring circuit-breaker and bulkhead policies maintain consistent state while handlers and HttpClient instances are recycled underneath them, has suggested they should be singleton per named HttpClient configuration on HttpClientFactory.

The stateful nature of circuit-breaker and bulkhead policies also has intended functional consequences when sharing policies across call sites:

Circuit-breaker: Sharing a CircuitBreakerPolicy instance at multiple call sites causes those call sites to share circuit statistics and state, and thus to break in common.

This is intended: for example, using the same CircuitBreakerPolicy instance for all calls to (various endpoints on) contoso.com means any call you place to any endpoint on contoso.com can know if other calls have already detected that contoso.com is 'down' (and broken the circuit).

Bulkhead: Calls placed through the same BulkheadPoliy instance share the capacity of that bulkhead.

The corollary of this for HttpClientFactory is:

Circuit-breaker: Where a named HttpClient configuration on an HttpClientFactory uses a CircuitBreakerPolicy, that named HttpClient should be usage-scoped to calls to endpoints the user wishes to break in common.

This is a good fit for the examples @rynowak / @glennc have given for HttpClientFactory, with eg a named client configuration of "github" being used with a BaseUri "github.com" (with the implication calls through it will be to somewhere on github.com).

Bulkhead: Similarly, a named HttpClient configuration, if configured with a BulkheadPolicy, should be usage-scoped to calls the user wishes to share that bulkhead capacity.

Reading through the Consumption Patterns Wiki page for how to use Refit has the following line of example code:

.AddTypedClient(c => Refit.RestService.For<IGitHubApi>());

This is incorrect as it does not pass the HttpClient (the c parameter in the lambda) to For<T>().

It should be:

.AddTypedClient(c => Refit.RestService.For<IGitHubApi>(c));

Our SetHandlerLifetime method is intended to accept a value of Timeout.InfiniteTimeSpan and interpret this as a gesture to disable the handler rotation feature. Unfortunately the validation logic is overzealous.

This is a bug and we need to allow this. We intended for this to work and even documented it 👍

Title

In the RetryHandler class, in the SendAsync method, when an exception occurs, a call to Task.Delay is made but the result is never used/awaited.

Functional impact

The sample code of RetryHandler will not wait for retries on error, instead it will instantly retry.

Further technical details

See code here: https://github.com/aspnet/HttpClientFactory/blob/dev/samples/HttpClientFactorySample/Program.cs#L108

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 #58 (comment)
• Do we need an opinionated retry-focused extension method #58 (comment)

For docs:

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

See: https://www.nuget.org/packages/Polly.Extensions.Http-Signed/

We should integrate this for RC.

This is a follow up from #51.

In that issue @andrewlock suggest log messages be prefixed System.Net.Http.HttpClient. rather than System.Net.Http..

@glennc agreed and @rynowak seems to suggest this was implemented but I don't see that in code?

Happy to submit a PR for the change if it is?

Currently when HttpClient times out, the user gets a TaskCanceledException. It would be a much better experience if a user got some sort of TimeoutException instead. Here's what we do today as an example:

catch (TaskCanceledException ex)
{
    if (cancellationToken.IsCancellationRequested)
    {
        exception = new HttpClientException("HttpClient request cancelled by token request.", ex);
    }
    else
    {
        exception = new TimeoutException("HttpClient request timed out. Timeout: " + builder.Inner.Timeout.TotalMilliseconds.Commify() + "ms", ex);
    }
}

cc @glennc

Polly Policy instances can be used across multiple call sites for intended functional outcomes. This is a good fit for named HttpClient configurations on HttpClientFactory with a BaseUri eg api.contoso.com indicating all calls will be to endpoints on api.contoso.com.

To complement re-use, Polly provides an execution-scoped Context which travels with every execution. This:

(a) provides a correlation id unique to each execution, allowing logging and telemetry to correlate the events particular to a single execution;

(b) distinguishes different call sites at which Policy instances may be being used, allowing logging and telemetry to correlate statistics particular to a single usage site/call path;

• For instance, it is typical to use the same circuit-breaker instance for all endpoints on api.contoso.com, but when analysing failures, it can be very useful to extract that call path A tended to be healthy but call path B tended to require retries, break circuits, etc.

(c) allows the functioning of CachePolicy. With CachePolicy, one typically configures a single CachePolicy instance, then Context supplies the key that the policy should use for caching items on that particular call path.

The execution-scoped Polly.Context is already made available to policy delegate hooks (eg onRetry, onBreak etc) and will also be available to forthcoming raw telemetry events.

Without HttpClientFactory doing anything around this, correlation ids (a) would be inconsistent in cases where multiple Polly policies are applied by layering DelegatingHandlers (each PolicyHttpMessageHandler layer would generate a fresh guid). And features (b) and (c) would not be available.

Options for supporting Polly.Context in PolicyHttpMessageHandler

Here within PolicyHttpMessageHandler we could:

Polly.Context pollyExecutionContext = request.Properties[PollyContext] ?? new Polly.Context(); // [1]
request.Properties[PollyContext] = pollyExecutionContext; // [2]  

return _policy.ExecuteAsync((context, token) => base.SendAsync(request, token), pollyExecutionContext, cancellationToken); // [3]

where:

private static readonly string PollyContext = "PollyExecutionContext"; // [4]

[1] and [2] ensure when there is a chain of delegating handlers, they all use the same context, fixing (a) above. [3] just despatches so that all Polly logging and telemetry has access to the Context.

If we changed [4] to:

public static readonly string PollyContext = "PollyExecutionContext"; // [4b]

then users who desired, would have this usage:

request.Properties[PolicyHttpMessageHandler.PollyContext] = new Polly.Context("MyUsageKey"); // [5] 
var response = client.DoWhateverAsync(request...); // (as usual)

This enables and fixes both use cases (b) and (c) above.

Stepping back: the issue here is that Polly users normally code the fooPolicy.ExecuteAsync(...) call directly, so can pass in a Context. In the HttpClientFactory integration, that call is (necessarily) within PolicyHttpMessageHandler.cs, so the user cannot directly pass in Context.

HttpClient being (presumably) not for modification, HttpRequestMessage.Properties seems like the intended(?)/available place for attaching execution-scoped data.

The extra line, [5], would not be necessary for ordinary usage. It would be an available option, for those who wanted the richer telemetry or to use CachePolicy.

Thoughts? Should this be part of the Polly-HttpClientFactory story? Are there any good alternatives?

This repos doesn't have the ./run.[sh|ps1] bootstrappers, let's update it.