[HttpClientFactory] UseSocketsHttpHandler API naming follow-up

[CarnaViire]

We need to reach consensus on whether we are fine with UseSocketsHttpHandler name for API introduced by #84075, or we want to rename it to Configure* or Set* or something else.

My opinion is that Configure*/Add*/Set* are logically wrong; my explanations are under the cut below. Also, Use* patern is very similar in core meaning to what ASP.NET Core uses in UseKestrel, UseHttpSys etc.

cc @davidfowl @halter73 @terrajobst @karelz @tekian

---

Explanation on why I chose not to use Configure/Add/Set in API name

1. ConfigureSocketsHttpHandler

I honestly believe such naming is confusing. HttpClientFactory doesn't have a concept of "SocketsHttpHandler" as a property that could be changed/configured. It has "Primary handler" and "Additional handlers" concepts. So you change/configure Primary handler, not "SocketsHttpHandler". Where did SocketsHttpHandler come from so we are configuring it? The name would look even stranger given that it is possible to not change anything in SocketsHttpHandler instance (by not passing a delegate):

// original proposal
services.AddHttpClient("foo")
    .UseSocketsHttpHandler();

// alternative
services.AddHttpClient("foo")
    .ConfigureSocketsHttpHandler(); // what and how is being configured? we didn't pass anything...

2. ConfigurePrimarySocketsHttpHandler

This is tiny bit better, as it contains the word "Primary", which hints that it is Named HttpClient's Primary handler that is being configured. However, now the naming is IMHO too similar to the existing ConfigurePrimaryHttpMessageHandler -- both start with "ConfigurePrimary...", end with "...Handler", contain "Http"... It is not end of the world, but you still need to make some effort to differentiate the two. And it still looks a bit strange if it's used without passing a delegate.

services.AddHttpClient("foo")
    .ConfigurePrimaryHttpMessageHandler(...); // which one is which? :)

services.AddHttpClient("foo")
    .ConfigurePrimarySocketsHttpHandler(...); // which one is which? :)

services.AddHttpClient("bar")
    .ConfigurePrimarySocketsHttpHandler(); // what and how is being configured? we didn't pass anything...

3. AddSocketsHttpHandler

While this is a decent alternative, IMHO, Add should only be used with collection-related things. If we add the handler, what is happening with the old one? is it staying? both will be called? 🙂

4. SetSocketsHttpHandler

This suffers from the same problem the first alternative had. We are setting Primary handler, not "SocketsHttpHandler".

Context from the discussion on #88864

@davidfowl wrote:

Why is it a Use vs Add/Configure?

@CarnaViire wrote:

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 wrote:

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 wrote:

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 wrote:

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 wrote:

To get a sense of why Use over Configure here, what else would we add a Use* extensions method on this API?

@CarnaViire wrote:

Potentially something related to a specific primary handler? e.g. UseWinHttpHandler(...) -> + configuration specific to WinHttpHandler

ScopeMismatch fix API could be named UseExistingScope(true) or UseHandlerScope(false)

Something like UseMinimalLogging(...) that I've described as a possible later expansion in #77312, to highlight it will replace existing logging, as opposed to Add...Logging, that only adds and you have to call RemoveAllLoggers beforehand

@davidfowl wrote:

Today we have Set* for that right? Here's what I see today:

builder.Services.AddHttpClient("Custom")
    .RedactLoggedHeaders(header => header.Equals("Authenticaton", StringComparison.OrdinalIgnoreCase))
    .ConfigureHttpClient(client => client.Timeout = TimeSpan.FromSeconds(10))
    .ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler()
    {
        AutomaticDecompression = System.Net.DecompressionMethods.GZip
    })
    .SetHandlerLifetime(TimeSpan.FromMinutes(5))
    .AddHttpMessageHandler<AuthenticationHander>();

You're suggesting we replace Set* with Use* because we're setting a property that is being configured (even that that's all configure* methods do right?), but because there's no arguments (or because it's optional).

After this change, it'll look like this:

builder.Services.AddHttpClient("Custom")
    .RedactLoggedHeaders(header => header.Equals("Authenticaton", StringComparison.OrdinalIgnoreCase))
    .ConfigureHttpClient(client => client.Timeout = TimeSpan.FromSeconds(10))
    .UseSocketsHttpHandler(h =>
    {
        h.PooledConnectionLifetime = TimeSpan.FromMinutes(5);
    })
    .SetHandlerLifetime(TimeSpan.FromMinutes(5))
    .AddHttpMessageHandler<AuthenticationHander>();

@CarnaViire wrote:

Today we have Set* for that right?

"obj.SetProperty(value)" translates into "obj.Property = value", doesn't it? If we choose Set* for the API, you would get "obj.value??? = ???" instead.... It's the same logical error IMHO as with Configure*

I'm not married to Use* in particular, I just believe Set*, Configure* and Add* are all logically wrong here.....

BTW doesn't ASP.NET have UseHttpSys(...), UseIIS(...), UseKestrel(...), UseQuic(...) for exactly same core meaning?

@davidfowl wrote:

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

[ghost]

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

Issue Details

---

We need to reach consensus on whether we are fine with UseSocketsHttpHandler name for API introduced by #84075, or we want to rename it to Configure* or Set* or something else.

cc @davidfowl @halter73 @terrajobst @karelz @tekian

---

Context from the discussion on #88864 (comment)

---- @davidfowl wrote:
Why is it a Use vs Add/Configure?

---- @CarnaViire wrote:
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 wrote:
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 wrote:
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 wrote:
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 wrote:
To get a sense of why Use over Configure here, what else would we add a Use* extensions method on this API?

---- @CarnaViire wrote:
Potentially something related to a specific primary handler? e.g. UseWinHttpHandler(...) -> + configuration specific to WinHttpHandler

ScopeMismatch fix API could be named UseExistingScope(true) or UseHandlerScope(false)

Something like UseMinimalLogging(...) that I've described as a possible later expansion in #77312, to highlight it will replace existing logging, as opposed to Add...Logging, that only adds and you have to call RemoveAllLoggers beforehand

---- @davidfowl wrote:
Today we have Set* for that right? Here's what I see today:

builder.Services.AddHttpClient("Custom")
    .RedactLoggedHeaders(header => header.Equals("Authenticaton", StringComparison.OrdinalIgnoreCase))
    .ConfigureHttpClient(client => client.Timeout = TimeSpan.FromSeconds(10))
    .ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler()
    {
        AutomaticDecompression = System.Net.DecompressionMethods.GZip
    })
    .SetHandlerLifetime(TimeSpan.FromMinutes(5))
    .AddHttpMessageHandler<AuthenticationHander>();

You're suggesting we replace Set* with Use* because we're setting a property that is being configured (even that that's all configure* methods do right?), but because there's no arguments (or because it's optional).

After this change, it'll look like this:

builder.Services.AddHttpClient("Custom")
    .RedactLoggedHeaders(header => header.Equals("Authenticaton", StringComparison.OrdinalIgnoreCase))
    .ConfigureHttpClient(client => client.Timeout = TimeSpan.FromSeconds(10))
    .UseSocketsHttpHandler(h =>
    {
        h.PooledConnectionLifetime = TimeSpan.FromMinutes(5);
    })
    .SetHandlerLifetime(TimeSpan.FromMinutes(5))
    .AddHttpMessageHandler<AuthenticationHander>();

---- @CarnaViire wrote:

Today we have Set* for that right?

"obj.SetProperty(value)" translates into "obj.Property = value", doesn't it? If we choose Set* for the API, you would get "obj.value??? = ???" instead.... It's the same logical error IMHO as with Configure*

I'm not married to Use* in particular, I just believe Set*, Configure* and Add* are all logically wrong here.....

BTW doesn't ASP.NET have UseHttpSys(...), UseIIS(...), UseKestrel(...), UseQuic(...) for exactly same core meaning?

---- @davidfowl wrote:
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

Author:	CarnaViire
Assignees:	CarnaViire
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[CarnaViire]

1. UseHttpSys() - Specify Http.Sys as the server to be used by the web host.
   UseHttpSys(...) - Specify Http.Sys as the server to be used by the web host and configure HttpSys options with the passed delegate.

2. UseKestrel() - Specify Kestrel as the server to be used by the web host.
   UseKestrel(...) - Specify Kestrel as the server to be used by the web host and configure Kestrel options with the passed delegate.

3. UseSocketsHttpHandler() - Specify SocketsHttpHandler as the primary handler to be used by the HttpClientFactory.
   UseSocketsHttpHandler(...) - Specify SocketsHttpHandler as the primary handler to be used by the HttpClientFactory and configure SocketsHttpHandler with the passed delegate.

Sounds pretty consistent to me 🙂

Also, interesting comparison, as there's
ConfigureKestrel(...) - Configures Kestrel options but does not register an IServer. -- the docs also forward to UseKestrel from here -- so my understanding is that ConfigureKestrel makes sense only after UseKestrel() is called. (Yes I know UseKestrel() was already called by default builder, but that is not the point)

[CarnaViire]

Expanding a little bit re ConfigureKestrel(...):

If HttpClientFactory either

1. Had a known and well-documented default of SocketsHttpHandler being the primary handler, or
2. Had a separate (parameterless?) API like UseSocketsHttpHandler() that you have to call before you call any configurational API on SocketsHttpHandler

-- in these two cases I would be fine with ConfigureSocketsHttpHandler(...) name. Because that would answer my question "Where did SocketsHttpHandler come from, so now we are configuring it?" These two cases (either known default or previous API call) bring SocketsHttpHandler into being a part of HttpClientFactory concept/state. And this is the same case with ConfigureKestrel(...).

[CarnaViire]

@davidfowl @halter73 any thoughts on this?

[karelz]

We are past the time we can make API changes for 8.0.
Given that there was no strong push back against current name choice, let's stick with that. Closing (no more follow up needed).

cc @davidfowl