dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.99k stars 4.66k forks source link

[docs] Typed HttpClient configuration is lost when a service is registered a second time #101884

Closed JakeYallop closed 3 weeks ago

JakeYallop commented 4 months ago

When a service containing a prior configured HttpClient is explicitly registered, any configuration for the client that service uses is then lost.

Consider the following simplified scenario. MyService is a service containing a HttpClient. Based on the configuration, I would expect MyService to be scoped and have a HttpClient with a rate limit of 1 request per second and have additional user agent headers. However, the rate limit nor the additional default request headers are applied.

In this simplified scenario, its obvious to see that the service has been registered twice. Removing the AddScoped<MyService> call means the service is registered as transient (from AddHttpClient<MyService>), and the http client configuration then applies as expected.

The primary message handler configured using ConfigureHttpClientDefaults is configured correctly, regardless of if there are multiple registrations of MyService.

var builder = Host.CreateDefaultBuilder();
builder.ConfigureServices(services =>
{
    services
        .ConfigureHttpClientDefaults(configure =>
        {
            configure
            .ConfigurePrimaryHttpMessageHandler(() => new MockDataHttpMessageHandler())
            .ConfigureHttpClient(x => x.DefaultRequestHeaders.UserAgent.Add(new("Test", "1.0")));
        })
        .AddHttpClient<MyService>(c => c.DefaultRequestHeaders.UserAgent.Add(new("ExplicitConfiguration", "1.0")))
        .ConfigureAdditionalHttpMessageHandlers((handlers, services) =>
        {
            var handler = new ClientSideRateLimitedHandler(new SlidingWindowRateLimiter(new()
            {
                AutoReplenishment = true,
                PermitLimit = 1,
                QueueLimit = int.MaxValue,
                Window = TimeSpan.FromSeconds(1),
                SegmentsPerWindow = 1
            }));
            handlers.Add(handler);
        })
        .Services
        .AddScoped<MyService>();
});

I'm not sure if there's a fix here, but I wanted to raise an issue for this as its very easy to make a mistake and accidentally remove all the configuration for a http client.

Output with misconfiguration (the mock data handler has a Task.Delay(100) in it):

Request completed. Time between request: 118ms
Test/1.0
Request completed. Time between request: 110ms
Test/1.0

Expected output:

Request completed. Time between request: 111ms
Test/1.0 ExplicitConfiguration/1.0
Request completed. Time between request: 980ms
Test/1.0 ExplicitConfiguration/1.0
Full runnable code listing ```xml ``` ```csharp using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using System.Diagnostics; using System.Globalization; using System.Net; using System.Threading.RateLimiting; var builder = Host.CreateDefaultBuilder(); builder.ConfigureServices(services => { services .ConfigureHttpClientDefaults(configure => { configure .RemoveAllLoggers() .ConfigurePrimaryHttpMessageHandler(() => new MockDataHttpMessageHandler()) .ConfigureHttpClient(x => x.DefaultRequestHeaders.UserAgent.Add(new("Test", "1.0"))); }) .AddHttpClient(c => c.DefaultRequestHeaders.UserAgent.Add(new("ExplicitConfiguration", "1.0"))) .ConfigureAdditionalHttpMessageHandlers((handlers, services) => { var handler = new ClientSideRateLimitedHandler(new SlidingWindowRateLimiter(new() { AutoReplenishment = true, PermitLimit = 1, QueueLimit = int.MaxValue, Window = TimeSpan.FromSeconds(1), SegmentsPerWindow = 1 })); handlers.Add(handler); }) .Services .AddHostedService() .AddScoped(); }); await builder.RunConsoleAsync(); public class MyService(HttpClient client) { private readonly HttpClient _client = client; public Task DoThingAsync() => _client.GetAsync("https://notarealurl.com"); } internal sealed class MockDataHttpMessageHandler : HttpMessageHandler { protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { Console.WriteLine(request.Headers.UserAgent.ToString()); return CreateHtmlResponseAsync(cancellationToken); } private static async Task CreateHtmlResponseAsync(CancellationToken cancellationToken) { var content = new HttpResponseMessage(HttpStatusCode.OK); content.Content = new StringContent("content"); await Task.Delay(100, cancellationToken).ConfigureAwait(false); return content; } } internal sealed class Run(MyService service) : BackgroundService { private readonly MyService _service = service; protected override async Task ExecuteAsync(CancellationToken stoppingToken) { var stopwatch = Stopwatch.StartNew(); while (!stoppingToken.IsCancellationRequested) { await _service.DoThingAsync(); Console.WriteLine($"Request completed. Time between request: {stopwatch.ElapsedMilliseconds}ms"); stopwatch.Restart(); } } } //https://learn.microsoft.com/en-us/dotnet/core/extensions/http-ratelimiter internal sealed class ClientSideRateLimitedHandler(RateLimiter limiter): DelegatingHandler, IAsyncDisposable { protected override async Task SendAsync( HttpRequestMessage request, CancellationToken cancellationToken) { using RateLimitLease lease = await limiter.AcquireAsync( permitCount: 1, cancellationToken); if (lease.IsAcquired) { return await base.SendAsync(request, cancellationToken); } var response = new HttpResponseMessage(HttpStatusCode.TooManyRequests); if (lease.TryGetMetadata( MetadataName.RetryAfter, out TimeSpan retryAfter)) { response.Headers.Add( "Retry-After", ((int)retryAfter.TotalSeconds).ToString( NumberFormatInfo.InvariantInfo)); } return response; } async ValueTask IAsyncDisposable.DisposeAsync() { await limiter.DisposeAsync().ConfigureAwait(false); Dispose(disposing: false); GC.SuppressFinalize(this); } protected override void Dispose(bool disposing) { base.Dispose(disposing); if (disposing) { limiter.Dispose(); } } } ```
dotnet-policy-service[bot] commented 4 months ago

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

dotnet-policy-service[bot] commented 4 months ago

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

CarnaViire commented 4 months ago

I would argue that overriding is an expected behavior in DI paradigm. But this scenario definitely will benefit from explicit documentation.

What's unfortunate here is that the "default" HttpClient (the one auto-added with string.Empty name) -- is also registered as a transient service https://github.com/dotnet/runtime/blob/9be628743487aac580a5e933f08d459dc50c80fb/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs#L65-L69

So, instead of failing, your not-anymore-"typed client" MyService ends up being created with this "default" HttpClient injected (which might be especially confusing because ConfigureHttpClientDefaults was applied to that client as well)

On a side note, I should warn you regarding the use of scopes with HttpClientFactory, there's a known limitation that different application scopes can end up sharing a single handler instance, for more information see https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory#message-handler-scopes-in-ihttpclientfactory and https://github.com/dotnet/runtime/issues/47091

JakeYallop commented 4 months ago

I would argue that overriding is an expected behavior in DI paradigm.

I agree with this - to me though, the name of the API suggests that I am configuring the HTTP client that is going to be injected into that service, rather than registering the service as well.

In fairness, its a mistake you only make once, and I've used typed clients before and not ran into such issues.

CarnaViire commented 1 week ago

Covered in https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory-troubleshooting#typed-client-is-registered-a-second-time