Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,22 @@
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Net.Http;
using System.Threading;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;

namespace Microsoft.Extensions.Http
{
internal sealed class DefaultHttpMessageHandlerBuilder : HttpMessageHandlerBuilder
{
private HttpMessageHandler? _primaryHandler;
private string? _name;

public DefaultHttpMessageHandlerBuilder(IServiceProvider services)
{
Services = services;
}

private string? _name;

[DisallowNull]
public override string? Name
{
Expand All @@ -28,7 +32,11 @@ public override string? Name
}
}

public override HttpMessageHandler PrimaryHandler { get; set; } = new HttpClientHandler();
public override HttpMessageHandler PrimaryHandler
{
get => _primaryHandler ??= CreatePrimaryHandler();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had concerns that there are certain workarounds like the one existing in gRPC, where the user sets the handler explicitly to null! (it was also discussed a bit here https://github.com/dotnet/runtime/pull/90272/files#r1293537394)

https://github.com/grpc/grpc-dotnet/blob/854e0fd3d0a7da8507ae0f4bf3adac129fb7e10b/src/Grpc.Net.ClientFactory/GrpcClientServiceExtensions.cs#L349-L354

I'll try to use grep app to check whether there are any other users with similar hacks; and I'll also chat with @JamesNK when he's back to see whether he would be able to change the hack to e.g. ConfigureHttpClientDefaults

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you don't need to worry about it anymore: grpc/grpc-dotnet#2445

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JamesNK!

set => _primaryHandler = value;
}

public override IList<DelegatingHandler> AdditionalHandlers { get; } = new List<DelegatingHandler>();

Expand All @@ -44,5 +52,36 @@ public override HttpMessageHandler Build()

return CreateHandlerPipeline(PrimaryHandler, AdditionalHandlers);
}

#pragma warning disable CA1822, CA1859 // Mark members as static, Use concrete types when possible for improved performance
private HttpMessageHandler CreatePrimaryHandler()
#pragma warning restore CA1822, CA1859
{
#if NET
// On platforms where SocketsHttpHandler is supported, HttpClientHandler is a thin wrapper
// around it. By using SocketsHttpHandler directly, we can avoid the overhead of the wrapper,
// but more importantly, we can configure it to limit the lifetime of its pooled connections
// to match the requested lifetime of the handler itself. That way, if/when someone holds on
// to a resulting HttpClient for a prolonged period of time, it'll still benefit from connection
// recycling, and without needing to tear down and reconstitute the rest of the handler pipeline.
if (SocketsHttpHandler.IsSupported)
{
SocketsHttpHandler handler = new();

if (Services.GetService<IOptionsMonitor<HttpClientFactoryOptions>>() is IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to think more about this part. I need to check whether it would give us an expected result in all circumstances. Also I need to make sure the _name is also always set to an expected value.

I'll return to it tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I still don't have an answer to that. It's in my pipeline, I will reply by Monday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the answers:

  • _name is set in the default factory right after creating the instance and before any of the actions are applied to the builder. So we're ok from this regard.
  • getting the options from IOptionsMonitor here in this point in time (while creating the handler pipeline) also aligns with the options usage in other places, including the default factory, so it's ok as well.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub Is the check is IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor only needed to verify whether the options could be resolved from the container in general? If so, the check is redundant, since the instance of DefaultHttpClientFactory was already created by this point, so IOptionsMonitor<HttpClientFactoryOptions> was already successfully injected into its constructor.

I'd vote for simply injecting IOptionsMonitor<HttpClientFactoryOptions> into DefaultHttpMessageHandlerBuilder's constructor here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the check is IOptionsMonitor optionsMonitor only needed to verify whether the options could be resolved from the container in general?

Mainly*, yes. It's guaranteed to have been registered? Where does that happen?

*Since IServiceProvider was being injected into the constructor rather than individual resources, this also seemed to be the more consistent approach, but I can change it if desired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since IServiceProvider was being injected into the constructor

It's there because we need to flow it to configuration callbacks, bc we don't really know what services the callbacks will need at this point.

It's guaranteed to have been registered? Where does that happen?

Yes, it happens in the "base" AddHttpClient() method which is called from all other AddHttpClient methods:


which in turn has
services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>)));

Constructor injection will align with the other usages in

IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor,

and
public LoggingHttpMessageHandlerBuilderFilter(IServiceProvider serviceProvider, IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor)

So I believe it will be better to do it the same way, that being said -- I don't want to hold back this PR only for that change; I can do it myself later as a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I believe it will be better to do it the same way, that being said -- I don't want to hold back this PR only for that change; I can do it myself later as a follow-up.

Ok, thanks. I started making the change, but it ends up adding a lot more ifdefs (only wanting a field for the options if this NET, only having the ctor parameter if it's NET, etc.), so I'll leave it up to you as a follow-up if you decide you still want to go that route.

{
TimeSpan lifetime = optionsMonitor.Get(_name).HandlerLifetime;
if (lifetime >= TimeSpan.Zero)
{
handler.PooledConnectionLifetime = lifetime;
}
}

return handler;
}
#endif

return new HttpClientHandler();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,16 @@ public void Ctor_SetsPrimaryHandler()
var builder = new DefaultHttpMessageHandlerBuilder(Services);

// Act
Assert.IsType<HttpClientHandler>(builder.PrimaryHandler);
#if NET
if (SocketsHttpHandler.IsSupported)
{
Assert.IsType<SocketsHttpHandler>(builder.PrimaryHandler);
}
else
#endif
{
Assert.IsType<HttpClientHandler>(builder.PrimaryHandler);
}
}

// Moq heavily utilizes RefEmit, which does not work on most aot workloads
Expand Down Expand Up @@ -78,7 +87,7 @@ public void Build_SomeAdditionalHandlers_PutsTogetherDelegatingHandlers()

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/50873", TestPlatforms.Android)]
public void Build_PrimaryHandlerIsNull_ThrowsException()
public void Build_PrimaryHandlerIsNull_UsesDefault()
{
// Arrange
var builder = new DefaultHttpMessageHandlerBuilder(Services)
Expand All @@ -87,8 +96,8 @@ public void Build_PrimaryHandlerIsNull_ThrowsException()
};

// Act & Assert
var exception = Assert.Throws<InvalidOperationException>(() => builder.Build());
Assert.Equal("The 'PrimaryHandler' must not be null.", exception.Message);
var handler = builder.Build();
Assert.NotNull(handler);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,41 @@ public void UseSocketsHttpHandler_Parameterless_Success()
var defaultPrimaryHandlerChain = messageHandlerFactory.CreateHandler("DefaultPrimaryHandler");
var socketsHttpHandlerChain = messageHandlerFactory.CreateHandler("SocketsHttpHandler");

Assert.IsType<HttpClientHandler>(GetPrimaryHandler(defaultPrimaryHandlerChain));
Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(defaultPrimaryHandlerChain));
Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(socketsHttpHandlerChain));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public void DefaultPrimaryHandler_RespectsHandlerLifetime()
{
var serviceCollection = new ServiceCollection();
serviceCollection.AddHttpClient();
serviceCollection.Configure<HttpClientFactoryOptions>(options => options.HandlerLifetime = TimeSpan.FromMinutes(42));

var services = serviceCollection.BuildServiceProvider();
var messageHandlerFactory = services.GetRequiredService<IHttpMessageHandlerFactory>();
var defaultPrimaryHandlerChain = messageHandlerFactory.CreateHandler();

SocketsHttpHandler handler = Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(defaultPrimaryHandlerChain));
Assert.Equal(TimeSpan.FromMinutes(42), handler.PooledConnectionLifetime);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public void DefaultPrimaryHandler_NamedClient_RespectsHandlerLifetime()
{
var serviceCollection = new ServiceCollection();
serviceCollection.AddHttpClient("Configured");
serviceCollection.Configure<HttpClientFactoryOptions>("Configured1", options => options.HandlerLifetime = TimeSpan.FromMinutes(42));
serviceCollection.Configure<HttpClientFactoryOptions>("Configured2", options => options.HandlerLifetime = TimeSpan.FromMinutes(84));

var services = serviceCollection.BuildServiceProvider();
var messageHandlerFactory = services.GetRequiredService<IHttpMessageHandlerFactory>();

Assert.Equal(TimeSpan.FromMinutes(42), Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(messageHandlerFactory.CreateHandler("Configured1"))).PooledConnectionLifetime);
Assert.Equal(TimeSpan.FromMinutes(84), Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(messageHandlerFactory.CreateHandler("Configured2"))).PooledConnectionLifetime);
Assert.Equal(TimeSpan.FromMinutes(2), Assert.IsType<SocketsHttpHandler>(GetPrimaryHandler(messageHandlerFactory.CreateHandler())).PooledConnectionLifetime);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public void UseSocketsHttpHandler_ConfiguredByAction_Success()
{
Expand All @@ -66,7 +97,7 @@ public void UseSocketsHttpHandler_ConfiguredByAction_Success()
var unconfiguredHandler = (SocketsHttpHandler)GetPrimaryHandler(unconfiguredHandlerChain);
var configuredHandler = (SocketsHttpHandler)GetPrimaryHandler(configuredHandlerChain);

Assert.Equal(Timeout.InfiniteTimeSpan, unconfiguredHandler.PooledConnectionLifetime);
Assert.Equal(TimeSpan.FromMinutes(2), unconfiguredHandler.PooledConnectionLifetime);
Assert.Equal(TimeSpan.FromMinutes(1), configuredHandler.PooledConnectionLifetime);
}

Expand Down