Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal] Configure SocketsHttpHandler for HttpClientFactory with IConfiguration #84075

Closed
tekian opened this issue Mar 29, 2023 · 17 comments · Fixed by #88864
Closed

[API Proposal] Configure SocketsHttpHandler for HttpClientFactory with IConfiguration #84075

tekian opened this issue Mar 29, 2023 · 17 comments · Fixed by #88864
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-HttpClientFactory blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@tekian
Copy link

tekian commented Mar 29, 2023

Original issue by @tekian

Background and motivation

The purpose of the new API, represented by the HttpClientSocketHandlingExtensions, SocketsHttpHandlerBuilder and SocketsHttpHandlerOptions classes, is to provide a more convenient and fluent way to configure SocketsHttpHandler instances for named HttpClient instances in applications that use dependency injection and the IHttpClientFactory.

API Proposal

public class SocketsHttpHandlerOptions
{
    public bool AllowAutoRedirect { get; set; }
    public bool UseCookies { get; set; }
    public int MaxConnectionsPerServer { get; set; }
    public DecompressionMethods AutomaticDecompression { get; set; }
    public TimeSpan ConnectTimeout { get; set; }
    public TimeSpan PooledConnectionLifetime { get; set; }
    public TimeSpan PooledConnectionIdleTimeout { get; set; }

#if NET5_0_OR_GREATER
    public TimeSpan KeepAlivePingDelay { get; set; }
    public TimeSpan KeepAlivePingTimeout { get; set; }
#endif
}

public class SocketsHttpHandlerBuilder
{
    public string Name { get; }
    public IServiceCollection Services { get; }
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<IServiceProvider, SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(IConfigurationSection section) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(Action<SocketsHttpHandlerOptions> configure) {}
}

public static class HttpClientSocketHandlingExtensions
{
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder) {}
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder, Action<SocketsHttpHandlerBuilder> configure) {}
}

API Usage

{
  "HttpClientSettings": {
    "MyClient": {
      "AllowAutoRedirect": true,
      "UseCookies": true,
      "ConnectTimeout": "00:00:05"
    }
  }
}
public void ConfigureServices(IServiceCollection services)
{
    IConfiguration configuration = ...; 

    services
        .AddHttpClient("MyClient")
        .AddSocketsHttpHandler(builder =>
        {
            builder.ConfigureOptions(configuration.GetSection("HttpClientSettings:MyClient"));
        });
}

Additionally, we could consider also adding following convenience methods:

public class SocketsHttpHandlerBuilder
{
       public SocketsHttpHandlerBuilder ConfigureClientCertificate(Func<IServiceProvider, X509Certificate2> clientCertificate) { ...}
       public SocketsHttpHandlerBuilder DisableRemoteCertificateValidation() { ... }
}

....either in this form or as an extension methods.

Alternative Designs

No response

Risks

No response


Background and motivation

The purpose of the new API, represented by the ISocketsHttpHandlerBuilder, SocketsHttpHandlerBuilderExtensions and UseSocketsHttpHandler, is to provide a more convenient and fluent way to configure SocketsHttpHandler instances for named HttpClient instances in applications that use dependency injection and the IHttpClientFactory. One of the main asks is to be able to configure SocketsHttpHandler via a configuration file.

This is a convenience API for some of the most widely used basic scenarios investigated by dotnet/extensions team.
While it is possible to achieve the same result by existing methods, the API significantly simplifies it in the common scenarios.

Note: this API is .NET 5+ only.

API Proposal

namespace Microsoft.Extensions.DependencyInjection;

// existing
public static class HttpClientBuilderExtensions
{
#if NET5_0_OR_GREATER

    // new
    [UnsupportedOSPlatform("browser")]
    public static IHttpClientBuilder UseSocketsHttpHandler(this IHttpClientBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider>? configureHandler = null) {}

    [UnsupportedOSPlatform("browser")]
    public static IHttpClientBuilder UseSocketsHttpHandler(this IHttpClientBuilder builder,
        Action<ISocketsHttpHandlerBuilder> configureBuilder) {}

#endif

    // existing (+2 overloads)
    // public static IHttpClientBuilder ConfigurePrimaryHttpMessageHandler(this IHttpClientBuilder builder, Func<HttpMessageHandler> configureHandler) {}
}

#if NET5_0_OR_GREATER

// new
public interface ISocketsHttpHandlerBuilder
{
    string Name { get; }
    IServiceCollection Services { get; }
}

// new
public static class SocketsHttpHandlerBuilderExtensions
{
    [UnsupportedOSPlatform("browser")]
    public static ISocketsHttpHandlerBuilder Configure(this ISocketsHttpHandlerBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider> configure) {}

    [UnsupportedOSPlatform("browser")]
    public static ISocketsHttpHandlerBuilder Configure(this ISocketsHttpHandlerBuilder builder,
        IConfigurationSection configurationSection) {}
}

#endif

API Usage

// uses SocketsHttpHandler as a primary handler
services.AddHttpClient("foo")
    .UseSocketsHttpHandler();

// sets up properties on the handler directly
services.AddHttpClient("bar")
    .UseSocketsHttpHandler((handler, _) =>
    {
        handler.PooledConnectionLifetime = TimeSpan.FromMinutes(2);
        handler.UseCookies = false;
        handler.MaxConnectionsPerServer = 1;
    });

// loads properties from config file and chains setting up SslOptions via builder
services.AddHttpClient("baz")
    .UseSocketsHttpHandler(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:baz"))
               .Configure((handler, _) => handler.SslOptions = new SslClientAuthenticationOptions
               {
                   RemoteCertificateValidationCallback = delegate { return true; },
               })
    );
{
  "HttpClientSettings": {
    "baz": {
      "AllowAutoRedirect": true,
      "UseCookies": false,
      "ConnectTimeout": "00:00:05"
    }
  }
}

Illustrative only: additional extension methods

services.AddHttpClient("qux")
    .UseSocketsHttpHandler(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:qux"))
            .SetMaxConnectionsPerServer(1)
            .DisableRemoteCertificateValidation()
    );

// e.g. added by user or a 3rd party lib
public static class MyFluentSocketsHttpHandlerBuilderExtensions
{
    public static ISocketsHttpHandlerBuilder DisableRemoteCertificateValidation(this ISocketsHttpHandlerBuilder builder, bool allowAutoRedirect) {}
    public static ISocketsHttpHandlerBuilder ConfigureClientCertificate(Func<IServiceProvider, X509Certificate> clientCertificate) {}

    // ...

    public static ISocketsHttpHandlerBuilder SetAllowAutoRedirect(this ISocketsHttpHandlerBuilder builder, bool allowAutoRedirect) {}
    public static ISocketsHttpHandlerBuilder SetUseCookies(this ISocketsHttpHandlerBuilder builder, bool useCookies) {}
    public static ISocketsHttpHandlerBuilder SetMaxConnectionsPerServer(this ISocketsHttpHandlerBuilder builder, int maxConnectionsPerServer) {}
    public static ISocketsHttpHandlerBuilder SetConnectTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan connectTimeout) {}
    public static ISocketsHttpHandlerBuilder SetPooledConnectionLifetime(this ISocketsHttpHandlerBuilder builder, TimeSpan pooledConnectionLifetime) {}
    public static ISocketsHttpHandlerBuilder SetPooledConnectionIdleTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan pooledConnectionIdleTimeout) {}
    public static ISocketsHttpHandlerBuilder SetKeepAlivePingDelay(this ISocketsHttpHandlerBuilder builder, TimeSpan keepAlivePingDelay) {}
    public static ISocketsHttpHandlerBuilder SetKeepAlivePingTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan keepAlivePingTimeout) {}

    // ...
}
@tekian tekian added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 29, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 29, 2023
@ghost
Copy link

ghost commented Mar 29, 2023

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

Issue Details

Background and motivation

The purpose of the new API, represented by the HttpClientSocketHandlingExtensions, SocketsHttpHandlerBuilder and SocketsHttpHandlerOptions classes, is to provide a more convenient and fluent way to configure SocketsHttpHandler instances for named HttpClient instances in applications that use dependency injection and the IHttpClientFactory.

API Proposal

public class SocketsHttpHandlerOptions
{
    public bool AllowAutoRedirect { get; set; }
    public bool UseCookies { get; set; }
    public int MaxConnectionsPerServer { get; set; }
    public DecompressionMethods AutomaticDecompression { get; set; }
    public TimeSpan ConnectTimeout { get; set; }
    public TimeSpan PooledConnectionLifetime { get; set; }
    public TimeSpan PooledConnectionIdleTimeout { get; set; }

#if NET5_0_OR_GREATER
    public TimeSpan KeepAlivePingDelay { get; set; }
    public TimeSpan KeepAlivePingTimeout { get; set; }
#endif
}

public class SocketsHttpHandlerBuilder
{
    public string Name { get; }
    public IServiceCollection Services { get; }
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<IServiceProvider, SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(IConfigurationSection section) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(Action<SocketsHttpHandlerOptions> configure) {}
}

public static class HttpClientSocketHandlingExtensions
{
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder) {}
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder, Action<SocketsHttpHandlerBuilder> configure) {}
}

API Usage

{
  "HttpClientSettings": {
    "MyClient": {
      "AllowAutoRedirect": true,
      "UseCookies": true,
      "ConnectTimeout": "00:00:05"
    }
  }
}
public void ConfigureServices(IServiceCollection services)
{
    IConfiguration configuration = ...; 

    services
        .AddHttpClient("MyClient")
        .AddSocketsHttpHandler(builder =>
        {
            builder.ConfigureOptions(configuration.GetSection("HttpClientSettings:MyClient"));
        });
}

Alternative Designs

No response

Risks

No response

Author: tekian
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: -

@CarnaViire
Copy link
Member

@tekian Thanks for the suggestions. But the problem is that SocketsHttpHandler only exists in .NET Core 2.1+. HttpClientFactory is built for netstandard2.0.

There're more problems to that -- I wonder how these APIs should behave on .NET Framework? On WASM or on Mobile platforms, where platform-specific handlers are used? Should it throw in these cases? Or ignore the configuration?

What should happen if a user configured their own PrimaryHandler in addition to calling this API? Should the configuration replace user's handler? Or the other way around?

@tekian
Copy link
Author

tekian commented Mar 30, 2023

@CarnaViire See inline.

But the problem is that SocketsHttpHandler only exists in .NET Core 2.1+.

Cannot the entire feature be available only for .NET Core 2.1+?

On WASM or on Mobile platforms, where platform-specific handlers are used? Should it throw in these cases? Or ignore the configuration?

It should be opt-in. Unless you call AddSocketsHttpHandler() everything would work as it does today.

What should happen if a user configured their own PrimaryHandler in addition to calling this API? Should the configuration replace user's handler? Or the other way around?

Then order matters. Whichever comes last should win. We could change the name of the method to say SetSocketsHttpHandler or UseSocketsHttpHandler to better reflect the semantics.

@ghost
Copy link

ghost commented Mar 31, 2023

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

Issue Details

Background and motivation

The purpose of the new API, represented by the HttpClientSocketHandlingExtensions, SocketsHttpHandlerBuilder and SocketsHttpHandlerOptions classes, is to provide a more convenient and fluent way to configure SocketsHttpHandler instances for named HttpClient instances in applications that use dependency injection and the IHttpClientFactory.

API Proposal

public class SocketsHttpHandlerOptions
{
    public bool AllowAutoRedirect { get; set; }
    public bool UseCookies { get; set; }
    public int MaxConnectionsPerServer { get; set; }
    public DecompressionMethods AutomaticDecompression { get; set; }
    public TimeSpan ConnectTimeout { get; set; }
    public TimeSpan PooledConnectionLifetime { get; set; }
    public TimeSpan PooledConnectionIdleTimeout { get; set; }

#if NET5_0_OR_GREATER
    public TimeSpan KeepAlivePingDelay { get; set; }
    public TimeSpan KeepAlivePingTimeout { get; set; }
#endif
}

public class SocketsHttpHandlerBuilder
{
    public string Name { get; }
    public IServiceCollection Services { get; }
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureHandler(Action<IServiceProvider, SocketsHttpHandler> configure) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(IConfigurationSection section) {}
    public SocketsHttpHandlerBuilder ConfigureOptions(Action<SocketsHttpHandlerOptions> configure) {}
}

public static class HttpClientSocketHandlingExtensions
{
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder) {}
    public static IHttpClientBuilder AddSocketsHttpHandler(this IHttpClientBuilder builder, Action<SocketsHttpHandlerBuilder> configure) {}
}

API Usage

{
  "HttpClientSettings": {
    "MyClient": {
      "AllowAutoRedirect": true,
      "UseCookies": true,
      "ConnectTimeout": "00:00:05"
    }
  }
}
public void ConfigureServices(IServiceCollection services)
{
    IConfiguration configuration = ...; 

    services
        .AddHttpClient("MyClient")
        .AddSocketsHttpHandler(builder =>
        {
            builder.ConfigureOptions(configuration.GetSection("HttpClientSettings:MyClient"));
        });
}

Alternative Designs

No response

Risks

No response

Author: tekian
Assignees: -
Labels:

api-suggestion, untriaged, area-Extensions-HttpClientFactory

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Apr 23, 2023

@CarnaViire could you please triage this issue and specify the plan addressing it? Thanks!

@CarnaViire
Copy link
Member

could you please triage this issue and specify the plan addressing it? Thanks!

We are still in the middle of internal discussions about where exactly and in what way to address this issue.
But from triage perspective I can put it to 8.0 already

@CarnaViire CarnaViire added this to the 8.0.0 milestone Apr 25, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 25, 2023
@tekian
Copy link
Author

tekian commented May 9, 2023

@CarnaViire, @rzikm I've added the two convenience methods to the proposal as discussed.

@stephentoub
Copy link
Member

stephentoub commented May 9, 2023

I suggest ditching the SocketsHttpHandlerOptions class and ConfigureOptions(Action<SocketsHttpHandlerOptions> configure) method. That represents only a subset of what's configurable on SocketsHttpHandler, and wanting to set something beyond that means you fall off a cliff and need to rewrite to use a different configuration approach. This also means that any time we add new options to SocketsHttpHandler, we have to mirror those properties on this separate options type for them to be configurable via that means. The type doesn't provide any additional expressive power over just setting the members on a SocketsHttpHandler instance itself, strictly less.

@rzikm
Copy link
Member

rzikm commented May 11, 2023

       public SocketsHttpHandlerBuilder ConfigureClientCertificate(Func<IServiceProvider, X509Certificate2> clientCertificate) { ...}
       public SocketsHttpHandlerBuilder DisableRemoteCertificateValidation()

I find it weird to have only those two available. Similarly to what stephentoub said above, those two expose only strict subset of what can be configured on the underlying SslStream, and any addition needs to be explicitly mirrored there. Wouldn't be better to have a callback for configuring SslClientAuthenticationOptions as a whole?

@tarekgh
Copy link
Member

tarekgh commented Jun 18, 2023

@CarnaViire what is the status of this issue? still tracked for .NET 8.0?

@CarnaViire
Copy link
Member

I am working on a PoC. I will finalize the API in the upcoming days.

still tracked for .NET 8.0?

Yes, we still plan it for 8.0

@dazinator
Copy link

dazinator commented Jun 21, 2023

Interesting. This may be tangential but I did a similar thing over here where the user configures a named options class with some high level properties, and then when the http client is built, I apply these options during its consturction.

These more simplistic options could be bound from config - but they span settings on the Handler, as well as on the HttpClient itself:-

services.AddHttpClient("foo-v1")
                    .ConfigureOptions((options) =>
                    {
                        options.BaseAddress = $"http://foo-v1.localhost";
                        options.EnableBypassInvalidCertificate = true;
                        options.MaxResponseContentBufferSize = 2000;
                        options.Timeout = TimeSpan.FromMinutes(2);
                        options.Handlers.Add("status-handler");
                    });                  
 /// <summary>
    /// Encapsulates options that can be applied to an <see cref="HttpClient"/>
    /// </summary>
    public class HttpClientOptions
    {
        public bool EnableBypassInvalidCertificate { get; set; } = false;

        public bool UseCookies { get; set; } = false;

        /// <summary>
        /// <see cref="HttpClient.BaseAddress"/>
        /// </summary>
        public string? BaseAddress { get; set; }
        /// <summary>
        /// <see cref="HttpClient.Timeout"/>
        /// </summary>
        public TimeSpan? Timeout { get; set; }
        /// <summary>
        /// <see cref="HttpClient.MaxResponseContentBufferSize"/>
        /// </summary>
        public long? MaxResponseContentBufferSize { get; set; }
        public List<string> Handlers { get; set; } = new List<string>();

        /// <summary>
        /// Apply these options to an <see cref="HttpClient"/>
        /// </summary>
        /// <param name="httpClient"></param>
        public virtual void Apply(HttpClient httpClient)
        {
            if (!string.IsNullOrWhiteSpace(BaseAddress))
            {
                httpClient.BaseAddress = new Uri(BaseAddress);
            }

            if (Timeout != null)
            {
                httpClient.Timeout = Timeout.Value;
            }

            if (MaxResponseContentBufferSize != null)
            {
                httpClient.MaxResponseContentBufferSize = MaxResponseContentBufferSize.Value;
            }
        }

    }

For example BaseAddress, MaxResponseContentBufferSize and Timeout are all configured on the HttpClient, where as to EnableBypassInvalidCertificate you need to configure changes at the handler level.

Where as the "Handlers" is an ordered list of handler names, that will be applied to the http client when it is built and these names correspond to a custom registry where the user can register handler factories with those names - and these facgtories will be invoked to create the handlers for the http client when it is built. The "name" of the http client being built is passed to these factories so they can bind to a json section or whatever to get the config to build the handler as specified for the particular named http client. This results in a config section like this:

{ 
"HttpClients": {
  "foo": {
      "BaseAddress": "http://foo.localhost",
      "Timeout": "00:00:30",
      "Handlers": [        
        "Diagnostics",
        "BasicAuth"
      ],
      "BasicAuth": {
        "Username": "MyUser",
        "Password": "ThisShouldBeASecret"
      }
    }
  }

It looks like this issue (from the title) is about configuring one specific Handler only, but what about the story for configuring a complete HttpClient as well as the constructuon of all of its handlers (I.e the definition of which handlers are constructed and the configuration applied to them) could be derived from config? This is what I have done over there because configuring just one fixed element of an Http client is not massively valuable - it's more valuable to be able to configure the complete HttpClient produced by the factory - i.e the HttpClient and the complete handler pipeline imho. This makes it possible for example to control which handlers are applied to a http client in order to do things like configure a diagnostics handler to be in the pipeline only on dev environments, or to switch out a BasicAuth handler for a JwtTokenAuth handler as a config change and not a code change.

@CarnaViire
Copy link
Member

Thanks for the insights @dazinator. This definitely has some interesting functionality, which can be explored in the future, would you mind creating a separate GH issue for that?

@CarnaViire
Copy link
Member

I've spent some time experimenting with the APIs and consulted @stephentoub. We aligned on the following API form:

// existing
public static class HttpClientBuilderExtensions
{
    // new

    public static IHttpClientBuilder UseSocketsHttpHandler(this IHttpClientBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider>? configureHandler = null) {}

    public static IHttpClientBuilder UseSocketsHttpHandler(this IHttpClientBuilder builder,
        Action<ISocketsHttpHandlerBuilder> configureBuilder) {}
}

// new
public interface ISocketsHttpHandlerBuilder
{
    string Name { get; }
    IServiceCollection Services { get; }
}

// new
public static class SocketsHttpHandlerBuilderExtensions
{
    public static ISocketsHttpHandlerBuilder Configure(this ISocketsHttpHandlerBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider> configure) {}

    public static ISocketsHttpHandlerBuilder Configure(this ISocketsHttpHandlerBuilder builder,
        IConfigurationSection configurationSection, bool reloadOnHandlerCreation = false) {}
}

The use of builder allows chaining configuration methods, and also allows for creation of additional extension configuration methods that could work directly with SocketsHttpHandler without the need to cast from HttpMessageHandler.

For example, while they most likely wouldn't meet the cut, we considered the following APIs (instead of SocketsHttpHandlerOptions proposed in the original post). We are not really fond of them either, because they just mimic what can already be done on SocketsHttpHandler directly, so this is mostly for illustrative purposes:

public static class SocketsHttpHandlerBuilderExtensions
{
    public static ISocketsHttpHandlerBuilder SetAllowAutoRedirect(this ISocketsHttpHandlerBuilder builder, bool allowAutoRedirect) {}
    public static ISocketsHttpHandlerBuilder SetUseCookies(this ISocketsHttpHandlerBuilder builder, bool useCookies) {}
    public static ISocketsHttpHandlerBuilder SetMaxConnectionsPerServer(this ISocketsHttpHandlerBuilder builder, int maxConnectionsPerServer) {}
    public static ISocketsHttpHandlerBuilder SetConnectTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan connectTimeout) {}
    public static ISocketsHttpHandlerBuilder SetPooledConnectionLifetime(this ISocketsHttpHandlerBuilder builder, TimeSpan pooledConnectionLifetime) {}
    public static ISocketsHttpHandlerBuilder SetPooledConnectionIdleTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan pooledConnectionIdleTimeout) {}
    public static ISocketsHttpHandlerBuilder SetKeepAlivePingDelay(this ISocketsHttpHandlerBuilder builder, TimeSpan keepAlivePingDelay) {}
    public static ISocketsHttpHandlerBuilder SetKeepAlivePingTimeout(this ISocketsHttpHandlerBuilder builder, TimeSpan keepAlivePingTimeout) {}
}

Usage examples:

// uses SocketsHttpHandler as a primary handler
services.AddHttpClient("foo")
    .UseSocketsHttpHandler();

// sets up properties on the handler directly
services.AddHttpClient("bar")
    .UseSocketsHttpHandler((handler, _) =>
    {
        handler.PooledConnectionLifetime = TimeSpan.FromMinutes(2);
        handler.UseCookies = false;
        handler.MaxConnectionsPerServer = 1;
    });

// loads properties from config and sets up properties via builder
services.AddHttpClient("baz")
    .UseSocketsHttpHandler(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:baz"))
            .Configure((handler, _) => handler.MaxConnectionsPerServer = 1)
    );

// illustrative only: additional extension methods
services.AddHttpClient("qux")
    .UseSocketsHttpHandler(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:qux"), reloadOnHandlerCreation: true)
            .SetMaxConnectionsPerServer(1)
            .DisableRemoteCertificateValidation()
    );

Re IConfigurationSection configuration API -- it will be possible to reload configuration in runtime @tekian. But it would be applied only when new primary handler is created. In default circumstances, it will be about each 2 minutes. However, it is directly tied to the handler lifetime, and if the user e.g. turns rotation off by setting handler lifetime to infinity, the updated configuration will never be applied. Same is when an HttpClient is captured in a singleton (consciously or by mistake). I suggest making reload opt-in by passing additional argument reloadOnHandlerCreation: true.

We've also agreed with @stephentoub that as we would not introduce a public SocketsHttpHandlerOptions class, we would read all simple properties of SocketsHttpHandler (not only those listed in SocketsHttpHandlerOptions). We consider also adding JSON schema in a way it was added for ASP.NET.

@dazinator
Copy link

dazinator commented Jun 26, 2023

@CarnaViire is there a reason the proposed api has to be specialised to sockets handler type only? It would be very useful to have a standard api to add and configure a handler in general.. and it looks on first glances that this proposal could be genericised?

services.AddHttpClient("qux")
    .UseHandler<SocketsHttpHandler>(builder =>
        builder.Configure(configuration.GetSection("HttpClientSettings:qux:sockets"), reloadOnHandlerCreation: true) // this reload business is confusing but left it 
            .SetMaxConnectionsPerServer(1)
            .DisableRemoteCertificateValidation()
    );

Where users (such as yours truly) would want to add and configure multiple other handlers they might not be in awe of having to create a specialised builder for each handler type. Thinking it would be good to have a similar experience to UseMiddleware on the server side.. you can add any middleware to the pipeline in a standard way. Middleware supports injecting IOptions though ofcourse and you've deviated away from Options so far.

Re IConfigurationSection configuration API -- it will be possible to reload configuration in runtime @tekian. But it would be applied only when new primary handler is created

Why would you want reloadOnHandlerCreation to be false? At the time handlers are created, if the config has changed you would always want that configuration to apply surely? If so I'd be tempted to drop this param as people will confuse it with some form of hot reload of the handler itself, when in reality it's just saying "please don't ignore my config changes when you next rebuild handlers". If a user did not want this then they could always supply a IConfiguration with fixed values that doesn't change at runtime?

Note: if you did decide to support some kind of hot reload of the handlers in future, i.e by actively invalidating the pooled handlers built with the old config when the config reload token was signalled, such that new handlers would be created for the next request, then I think adding a param such as reloadHandlerOnConfigChange would come in valuable as users may not want handlers to be hot reloaded and may be happy with current behaviour to let them lapse based on the lifetime settings - depending on the app and use case.. at that point it's good if you don't already have another setting around reloads to confuse people?

@CarnaViire
Copy link
Member

@dazinator thanks for confirming that the reload parameter is confusing... we decided to drop it altogether (for this release at least. It will be possible to add it later).

As for its default value, we had a meeting offline and aligned that the reload being off by default is more preferable, as it being on adds a performance overhead, which we don't want to have by default. My understanding is also that we'd like to promote immutable deployment. @tekian would be able to explain in more details.

If there would be enough ask in the future either for this, or for what you called "hot-reload", we will prioritize it accordingly.

@CarnaViire CarnaViire changed the title [API Proposal]: Add support to configure SocketsHttpHandler for named HttpClient instances with IOptions [API Proposal] Configure SocketsHttpHandler for HttpClientFactory with IConfiguration Jun 30, 2023
@CarnaViire CarnaViire added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 4, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 13, 2023

Video

  • We shouldn't take IConfigurationSection but IConfiguration (which IConfigurationSection extends)
namespace Microsoft.Extensions.DependencyInjection;

public static partial class HttpClientBuilderExtensions
{
#if NET5_0_OR_GREATER
    [UnsupportedOSPlatform("browser")]
    public static IHttpClientBuilder UseSocketsHttpHandler(
        this IHttpClientBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider>? configureHandler = null);

    [UnsupportedOSPlatform("browser")]
    public static IHttpClientBuilder UseSocketsHttpHandler(
        this IHttpClientBuilder builder,
        Action<ISocketsHttpHandlerBuilder> configureBuilder);
#endif
}

#if NET5_0_OR_GREATER

public interface ISocketsHttpHandlerBuilder
{
    string Name { get; }
    IServiceCollection Services { get; }
}

public static class SocketsHttpHandlerBuilderExtensions
{
    [UnsupportedOSPlatform("browser")]
    public static ISocketsHttpHandlerBuilder Configure(
        this ISocketsHttpHandlerBuilder builder,
        Action<SocketsHttpHandler, IServiceProvider> configure);

    [UnsupportedOSPlatform("browser")]
    public static ISocketsHttpHandlerBuilder Configure(
        this ISocketsHttpHandlerBuilder builder,
        IConfiguration configuration);
}

#endif

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 13, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2023
CarnaViire added a commit that referenced this issue Jul 17, 2023
#88864)

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
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 17, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-HttpClientFactory blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants