Configure SocketsHttpHandler for HttpClientFactory with IConfiguration

[CarnaViire]

Adds UseSocketsHttpHandler extension method for IHttpClientBuilder to set SocketsHttpHandler as a primary handler for a named HttpClient. Adds ISocketsHttpHandlerBuilder to configure the handler from IConfiguration and/or via a callback.

The API is .NET 5.0+ only.

Fixes #84075

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Adds UseSocketsHttpHandler extension method for IHttpClientBuilder to set SocketsHttpHandler as a primary handler for a named HttpClient. Adds ISocketsHttpHandlerBuilder to configure the handler from IConfiguration and/or via a callback.

The API is .NET 5.0+ only.

Fixes #84075

Author:	CarnaViire
Assignees:	-
Labels:	area-Extensions-HttpClientFactory
Milestone:	-

[ManickaP]

LGTM, but I'm not an expert here.

[davidfowl]

Why is it a Use vs Add/Configure?

[CarnaViire]

Because then it would have been ConfigurePrimaryHandlerToBeSocketsHttpHandler 😅

Point is, you don't have SocketsHttpHandler in HttpClientFactory by default - you have some PrimaryHandler, so if you'd say ConfigureSocketsHttpHandler, the question is -- where did it come from, so now we're configuring it?

So to me, saying UseSocketsHttpHandler was much clearer in explaining what happens - after you call it, the factory would be using SocketsHttpHandler in this named client (and you might decide not to configure it, if you don't specify a delegate)

[davidfowl]

ConfigureSocketsHttpHandler would be a better name. We don't have any other APIs that are Use at this level, I'm not sure why we'd introduce this verb when it's calling a Configure* method.

[CarnaViire]

While I agree that "Use" is very rarely used, I don't think that ConfigureSocketsHttpHandler is better, for the reason I described above

when it's calling a Configure* method

It's calling ConfigurePrimaryHttpMessageHandler method. "Primary" is an important piece of information in relation to "Configure" verb, as Primary handler is part of HttpClientFactory's "concept", it's a "property" on an HttpClient's configuration.

Primary handler property is a "left-hand operand" in the assignment, and SocketsHttpHandler value is a "right-hand operand", so to say.

But that's my opinion; I've sent an email to the API review board to discuss this further.

[halter73]

Given the example usage, I'm not sure I like "Configure..." given the nested calls to Configure( in the builder callback.

services.AddHttpClient("baz")
    .ConfigureSocketsHttpHandler(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:baz"))
               .Configure((handler, _) => handler.SslOptions = new SslClientAuthenticationOptions
               {
                   RemoteCertificateValidationCallback = delegate { return true; },
               })
    );

@davidfowl During API review, I thought your feedback would be to have it return the ISocketsHttpHandlerBuilder to reduce nesting. But if we did that, "Use..." would feel more out of place. "Add..." would be far more conventional given things like AddAuthentication and AddMvc, but it is a little weird that add really means replace in this case. But for completeness, the builder-returning API would look something like this:

services.AddHttpClient("baz")
    .AddSocketsHttpHandler()
    .Configure(configuration.GetSection("HttpClientSettings:baz"))
    .Configure((handler, _) => handler.SslOptions = new SslClientAuthenticationOptions
    {
        RemoteCertificateValidationCallback = delegate { return true; },
    });

But as @CarnaViire pointed out in API review, this could be annoying if you need the IHttpClientBuilder for anything else, you'd need to store the builder in a variable unless AddSocketsHttpHandler is the last part of the chain, so we decided against it.

var httpClientBuilder = services.AddHttpClient("baz");

httpClientBuilder.AddSocketsHttpHandler()
    .Configure(configuration.GetSection("HttpClientSettings:baz"))
    .Configure((handler, _) => handler.SslOptions = new SslClientAuthenticationOptions
    {
        RemoteCertificateValidationCallback = delegate { return true; },
    });

httpClientBuilder.SomethingElseThatReturnsABuilder()
    .DoSome()
    .MoreThings();

[davidfowl]

Use(this IWebHostBuilder) - Adding a combination of services, logging and configuration
Use(this IApplicationBuilder) - Adding middleware to the pipeline
Map(this IEndpointRouteBuilder) - Adding a route
Add(this IServiceCollection) - Adding services to the DI container
Configure(this IServiceCollecton) - Configuring options for T

[karelz]

It seems there is still ongoing naming discussion.
Given the timelines, I asked @CarnaViire to check in current state and then leave potential name change for later once we have consensus.
That way, majority of the work will get into customers' hands in Preview7 and we have a chance to get a feedback. We can easily change the public API name afterwards as P7 is NOT go-live.

[davidfowl]

I don't have a problem with that.

[davidfowl]

@CarnaViire can you open a follow up issue?

[CarnaViire]

@davidfowl #89023 (comment)

[CarnaViire]

All the CI failures are unrelated Known Issues.

[antonfirsov]

LGTM with a few suggestions.

[CarnaViire]

@antonfirsov per agreement with @karelz I'm merging this as-is to avoid CI delays; I will address your comments in a separate PR.