Make SocketsHttpHandler the default primary handler from HttpClientFactory

[stephentoub]

Contributes to #35987 (this makes SocketsHttpHandler the default handler but it doesn't disable or push down the higher-level rotation)

@CarnaViire, how do you feel about this change?

Today the default handler is HttpClientHandler, which is just a wrapper around SocketsHttpHandler on platforms where SocketsHttpHandler is supported. This means that the configuration possible on SocketsHttpHandler isn't available as part of the default handling, which means consumers get the default PooledConnectionLifetime of infinite. The lack of that was one of the main motivations behind HttpClientFactory's handler lifetime and handler recycling. Instead of constructing an HttpClientHandler as the default handler, we can construct a SocketsHttpHandler as the default handler, and we can set its PooledConnectionLifetime to match the HttpClientFactoryOptions.HandlerLifetime, whether its default of 2 minutes or whatever a user configured it to be. This in turn means that if code gets and holds on to a handler/client from the factory for a prolonged period of time, it's still getting the connection recycling according to its configured options.

[dotnet-policy-service]

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

[CarnaViire]

Sorry I was on vacation; I plan to review this today or tomorrow.

I remember thinking about the similar change but I don't remember from the top of my head what stopped me from proceeding with it. Maybe I intended to completely solve #35987 and it was not straightforward. Or maybe the fact that this is technically a breaking change. I'll comment when I brush up my memory 😄

[stephentoub]

@CarnaViire, did you have any other thoughts on this? Thanks.

[CarnaViire]

The performance concern I had: by setting PooledConnectionLifetime to the same value as HandlerLifetime, we are increasing the number of created connections. In addition, the connections that were newly created in the SocketsHttpHandler just as it was retired by the HttpClientFactory, would go to waste.

However, when I measure RPS with a slightly modified HttpClient benchmark (from aspnet/benchmarks), the difference seems negligible for small GET requests, and, quite surprisingly, for HTTP/1.1 GET 1Mb, RPS is even higher for the combination of PooledConnectionLifetime + HandlerLifetime than on just the HandlerLifetime rotation. I'm not sure I have an explanation for that atm. For HTTP/2.0 the difference is minimal even for GET 1Mb.

HttpClientFactory (HL=5s) vs (HL=5s, PCL=5s)

// HL = HandlerLifetime (HttpClientFactory rotation)
// PCL = PooledConnectionLifetime

.NET Core SDK Version = 8.0.301
Processor Count = 4 // (cpuSet="0-3")
profile = aspnet-citrine-lin
concurrency = 1000

Example parameters:

crank --config C:\Users\knatalia\dev\git\Benchmarks\scenarios\httpclient.benchmarks.yml --scenario httpclient-kestrel-get --profile aspnet-citrine-lin --client.cpuSet "0-3" --variable collectRequestTimings=true --variable responseSize=1048576 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=1000 --variable useHttps=true --variable httpVersion=1.1 --variable useHttpClientFactory=true --variable pooledConnectionLifetime=5 --variable hcfHandlerLifetime=5 --chart --json hcf-11-1000-1mb-5-5.json

HTTP/1.1 GET 128b

HTTP/1.1 GET 128b x 1000 threads	HL=5s	HL=5s, PCL=5s
Requests	1,648,740	1,645,577	-0.19%
Mean RPS	109,959	109,286	-0.61%
Time to response headers (ms) - p50	7.69	7.85	+2.08%
Time to response headers (ms) - p75	7.94	8.04	+1.18%
Time to response headers (ms) - p90	8.77	8.62	-1.71%
Time to response headers (ms) - p99	21.13	18.47	-12.57%
Time to last response content byte (ms) - p50	7.69	7.85	+2.08%
Time to last response content byte (ms) - p75	7.94	8.04	+1.18%
Time to last response content byte (ms) - p90	8.77	8.62	-1.71%
Time to last response content byte (ms) - p99	21.24	18.49	-12.96%
Current Http 1.1 Connections - Mean	1,591	1,747	+9.80%
Current Http 1.1 Connections - Max	2,922	2,816	-3.63%
HTTP 1.1 Requests Queue Duration (ms) - Mean	63	64	+2.78%
HTTP 1.1 Requests Queue Duration (ms) - Max	621	466	-24.96%
Current Outgoing Connect Attempts - Mean	19	15	-21.60%
Current Outgoing Connect Attempts - Max	178	152	-14.61%
Outgoing Connections Established - Mean	6,049	6,100	+0.85%
Outgoing Connections Established - Max	7,957	8,002	+0.57%
Total TLS handshakes completed	3,169	3,244	+2.37%
Current TLS handshakes - Mean	72	60	-16.64%
Current TLS handshakes - Max	585	358	-38.80%
TLS 1.3 Sessions Active - Mean	1,389	1,612	+16.02%
TLS 1.3 Sessions Active - Max	2,924	2,817	-3.66%
TLS 1.3 Handshake Duration (ms) - Mean	72	72	+1.00%
TLS 1.3 Handshake Duration (ms) - Max	509	415	-18.45%
Max CPU Usage (%)	100	100	0.00%
Max Working Set (MB)	1,057	925	-12.49%
Max Private Memory (MB)	2,331	2,193	-5.92%

HTTP/2.0 GET 128b

HTTP/2.0 GET 128b x 1000 threads	HL=5s	HL=5s, PCL=5s
Requests	3,198,753	3,163,237	-1.11%
Mean RPS	212,953	210,611	-1.10%
Time to response headers (ms) - p50	3.96	4.16	+5.13%
Time to response headers (ms) - p75	5.52	5.58	+1.22%
Time to response headers (ms) - p90	7.65	7.39	-3.42%
Time to response headers (ms) - p99	18.48	17.45	-5.60%
Time to last response content byte (ms) - p50	3.96	4.16	+5.13%
Time to last response content byte (ms) - p75	5.52	5.58	+1.22%
Time to last response content byte (ms) - p90	7.65	7.39	-3.42%
Time to last response content byte (ms) - p99	18.48	17.45	-5.60%
Current Http 2.0 Connections - Mean	24	23	-5.94%
Current Http 2.0 Connections - Max	52	49	-5.77%
HTTP 2.0 Requests Queue Duration (ms) - Mean	1	1	+3.55%
HTTP 2.0 Requests Queue Duration (ms) - Max	14	10	-33.84%
Current Outgoing Connect Attempts - Mean	0	0
Current Outgoing Connect Attempts - Max	0	0
Outgoing Connections Established - Mean	89	87	-1.96%
Outgoing Connections Established - Max	117	116	-0.85%
Total TLS handshakes completed	52	52	0.00%
Current TLS handshakes - Mean	0	0
Current TLS handshakes - Max	0	0
TLS 1.3 Sessions Active - Mean	24	23	-5.94%
TLS 1.3 Sessions Active - Max	52	49	-5.77%
TLS 1.3 Handshake Duration (ms) - Mean	3	2	-38.25%
TLS 1.3 Handshake Duration (ms) - Max	9	11	+13.95%
Max CPU Usage (%)	99	99	0.00%
Max Working Set (MB)	1,223	1,134	-7.28%
Max Private Memory (MB)	2,384	2,274	-4.61%

HTTP/1.1 GET 1Mb

HTTP/1.1 GET 1Mb x 1000 threads	HL=5s	HL=5s, PCL=5s
Requests	21,125	28,231	+33.64%
Mean RPS	1,442	1,884	+30.72%
Time to response headers (ms) - p50	109.04	163.70	+50.13%
Time to response headers (ms) - p75	391.79	383.53	-2.11%
Time to response headers (ms) - p90	754.37	1,100.94	+45.94%
Time to response headers (ms) - p99	1,887.20	3,508.78	+85.93%
Time to last response content byte (ms) - p50	587.94	237.40	-59.62%
Time to last response content byte (ms) - p75	1,022.01	515.17	-49.59%
Time to last response content byte (ms) - p90	1,468.89	1,203.11	-18.09%
Time to last response content byte (ms) - p99	2,638.38	4,177.45	+58.33%
Current Http 1.1 Connections - Mean	1,983	1,776	-10.41%
Current Http 1.1 Connections - Max	3,580	2,587	-27.74%
HTTP 1.1 Requests Queue Duration (ms) - Mean	122	128	+5.02%
HTTP 1.1 Requests Queue Duration (ms) - Max	552	845	+53.07%
Current Outgoing Connect Attempts - Mean	80	165	+107.38%
Current Outgoing Connect Attempts - Max	624	893	+43.11%
Outgoing Connections Established - Mean	5,219	5,381	+3.11%
Outgoing Connections Established - Max	6,717	7,158	+6.57%
Total TLS handshakes completed	3,054	3,001	-1.74%
Current TLS handshakes - Mean	99	96	-3.52%
Current TLS handshakes - Max	628	588	-6.37%
TLS 1.3 Sessions Active - Mean	1,457	1,014	-30.40%
TLS 1.3 Sessions Active - Max	3,054	2,587	-15.29%
TLS 1.3 Handshake Duration (ms) - Mean	779	1,135	+45.68%
TLS 1.3 Handshake Duration (ms) - Max	2,921	3,926	+34.39%
Max CPU Usage (%)	100	100	0.00%
Max Working Set (MB)	834	780	-6.47%
Max Private Memory (MB)	2,127	2,005	-5.74%

HTTP/2.0 GET 1Mb

HTTP/2.0 GET 1Mb x 1000 threads	HL=5s	HL=5s, PCL=5s
Requests	26,029	26,471	+1.70%
Mean RPS	1,743	1,753	+0.61%
Time to response headers (ms) - p50	41.77	42.93	+2.78%
Time to response headers (ms) - p75	64.30	66.35	+3.19%
Time to response headers (ms) - p90	227.46	214.12	-5.87%
Time to response headers (ms) - p99	345.51	315.33	-8.73%
Time to last response content byte (ms) - p50	606.98	605.84	-0.19%
Time to last response content byte (ms) - p75	708.67	727.38	+2.64%
Time to last response content byte (ms) - p90	832.27	855.53	+2.80%
Time to last response content byte (ms) - p99	1,139.89	1,101.62	-3.36%
Current Http 2.0 Connections - Mean	17	16	-4.35%
Current Http 2.0 Connections - Max	31	27	-12.90%
HTTP 2.0 Requests Queue Duration (ms) - Mean	84	81	-3.08%
HTTP 2.0 Requests Queue Duration (ms) - Max	355	303	-14.61%
Current Outgoing Connect Attempts - Mean	0	0
Current Outgoing Connect Attempts - Max	0	0
Outgoing Connections Established - Mean	56	56	-0.48%
Outgoing Connections Established - Max	70	69	-1.43%
Total TLS handshakes completed	31	30	-3.23%
Current TLS handshakes - Mean	0	0	0.00%
Current TLS handshakes - Max	1	1	0.00%
TLS 1.3 Sessions Active - Mean	17	16	-3.56%
TLS 1.3 Sessions Active - Max	31	27	-12.90%
TLS 1.3 Handshake Duration (ms) - Mean	13	9	-28.82%
TLS 1.3 Handshake Duration (ms) - Max	133	105	-20.95%
Max CPU Usage (%)	100	100	0.00%
Max Working Set (MB)	1,245	1,266	+1.69%
Max Private Memory (MB)	2,432	2,478	+1.89%

UPD: JFYI the the effect of HttpClientFactory overhead compared to a static HttpClient:

		Mean RPS: Static, PCL=5s	Mean RPS: Factory, HL=5s, PCL=5s
HTTP/1.1 GET	128b	122,151	109,286	-10.53%
HTTP/2.0 GET	128b	229,052	210,611	-8.05%
HTTP/1.1 GET	1Mb	2,191	1,839	-16.05%
HTTP/2.0 GET	1Mb	1,768	1,753	-0.86%

As an example:

HTTP/1.1 GET 1Mb x 1000 threads	Static, PCL=5s	Factory, HL=5s, PCL=5s
Requests	30,936	27,383	-11.49%
Mean RPS	2,191	1,839	-16.05%
Time to response headers (ms) - p50	134.16	148.90	+10.98%
Time to response headers (ms) - p75	307.83	428.48	+39.19%
Time to response headers (ms) - p90	1,191.74	1,058.33	-11.19%
Time to response headers (ms) - p99	3,792.85	3,498.23	-7.77%
Time to last response content byte (ms) - p50	172.74	226.85	+31.32%
Time to last response content byte (ms) - p75	375.57	537.72	+43.17%
Time to last response content byte (ms) - p90	1,296.53	1,327.62	+2.40%
Time to last response content byte (ms) - p99	3,880.83	4,067.77	+4.82%
Current Http 1.1 Connections - Mean	647	1,823	+181.76%
Current Http 1.1 Connections - Max	990	2,609	+163.54%
HTTP 1.1 Requests Queue Duration (ms) - Mean	91	116	+27.40%
HTTP 1.1 Requests Queue Duration (ms) - Max	586	786	+34.21%
Current Outgoing Connect Attempts - Mean	136	162	+18.90%
Current Outgoing Connect Attempts - Max	873	859	-1.60%
Outgoing Connections Established - Mean	4,008	5,366	+33.88%
Outgoing Connections Established - Max	4,937	7,092	+43.65%
Total TLS handshakes completed	1,916	3,594	+87.58%
Current TLS handshakes - Mean	26	65	+148.02%
Current TLS handshakes - Max	76	393	+417.11%
TLS 1.3 Sessions Active - Mean	642	1,635	+154.48%
TLS 1.3 Sessions Active - Max	990	2,573	+159.90%
TLS 1.3 Handshake Duration (ms) - Mean	799	1,172	+46.62%
TLS 1.3 Handshake Duration (ms) - Max	4,076	4,024	-1.28%
Max CPU Usage (%)	100	100	0.00%
Max Working Set (MB)	661	744	+12.56%
Max Private Memory (MB)	1,925	1,983	+3.01%

[CarnaViire]

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

[JamesNK]

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

[CarnaViire]

Thanks @JamesNK!

[CarnaViire]

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.

[CarnaViire]

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

[CarnaViire]

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.

👍

[CarnaViire]

@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.

[stephentoub]

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.

[CarnaViire]

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:

runtime/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs

Line 31 in c6bfb06

services.AddOptions();

which in turn has

runtime/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs

Line 28 in c6bfb06

services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>)));

Constructor injection will align with the other usages in

runtime/src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs

Line 65 in c6bfb06

IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor,

and

runtime/src/libraries/Microsoft.Extensions.Http/src/Logging/LoggingHttpMessageHandlerBuilderFilter.cs

Line 23 in c6bfb06

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.

[stephentoub]

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.

[CarnaViire]

On a side note, re: breaking change.

Any code that relied on the fact that the default handler was HttpClientHandler will break. For example, we even have such a code in the tests:

runtime/src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/DependencyInjection/HttpClientFactoryServiceCollectionExtensionsTest.cs

Lines 1381 to 1384 in 0cc0791

	.ConfigurePrimaryHttpMessageHandler((primaryHandler, _) =>
	{
	((HttpClientHandler)primaryHandler).Credentials = testCredentials;
	});

(the only reason the test didn't fail is that it uses a mock of HttpMessageHandlerBuilder instead of a real one)

[stephentoub]

On a side note, re: breaking change.

Any code that relied on the fact that the default handler was HttpClientHandler will break. For example, we even have such a code in the tests:

runtime/src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/DependencyInjection/HttpClientFactoryServiceCollectionExtensionsTest.cs

Lines 1381 to 1384 in 0cc0791

	.ConfigurePrimaryHttpMessageHandler((primaryHandler, _) =>
	{
	((HttpClientHandler)primaryHandler).Credentials = testCredentials;
	});

(the only reason the test didn't fail is that it uses a mock of HttpMessageHandlerBuilder instead of a real one)

Are we concerned about this actually affecting real code? I'd have thought we'd actively discourage folks relying on the pipeline defaulting to a specific never-changing configuration where they could downcast without fear of exception. That's similar to saying a virtual method typed to return Base and happens to actually return Derived1 today can't be changed to return Derived2 tomorrow (and we do that).

[CarnaViire]

Are we concerned about this actually affecting real code? I'd have thought we'd actively discourage folks relying on the pipeline defaulting to a specific never-changing configuration where they could downcast without fear of exception. That's similar to saying a virtual method typed to return Base and happens to actually return Derived1 today can't be changed to return Derived2 tomorrow (and we do that).

Thinking more about this, I agree. The fact that HttpClientHandler is used by default is clearly an implementation detail, and it doesn't seem to be mentioned anywhere in the docs, which is good. I did a research anyway, and I wasn't able to find the real cases of such casts. So I "remove" this concern from my list as well, which means we should be good to go

[CarnaViire]

LGTM, thanks!! 🥳