Consider updating HttpClientFactory defaults to leverage SocketsHttpHandler

[rynowak]

For .NET 5 we should switch the defaults of the factory:

• configure a connection lifetime for SocketsHttpHandler
• disable handler rotation

The problem that's solved by the current behavior (handler rotation) is solved more elegantly by just using the feature for it.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[CarnaViire]

Triage:
We cannot disable/remove handler rotation entirely because of 2 reasons:

• it will have to stay for .NET Framework
• it will have to stay for custom primary handlers as it might not be backed by SocketsHttpHandler at all, e.g. WinHttpHandler

We may still use SocketsHttpHandler for default primary handlers on .NET Core, but the benefit will not be as big (we cannot remove the rotation entirely).

Moving to future.

[stephentoub]

We may still use SocketsHttpHandler for default primary handlers on .NET Core

We really should do this. Someone should be able to create and hold on to an HttpClient and have things "just work". The fact that it doesn't today is arguably a bug. With this, at least the important case will "just work".

we cannot remove the rotation entirely

We can when the default handler is used on core, which is the most important case.

[stephentoub]

We should also consider pushing the rotation down into a delegating handler. When the primary handler isn't SHH, we can wrap the primary in a handler that will do the rotation, rather than doing it as part of the factory. That way, someone holding on to an HttpClient will still get the rotation under the covers.

[CarnaViire]

@stephentoub I agree. Both this issue and #47091 are crucial to overcome the most common fallouts. The good thing is that the API prerequisites that they both required are already done within #87914.

We've discussed some (potentially ugly 😅) ways to remove rotation from .NET Framework/mobile/browser as well. So it might boil down to just custom primary handlers.

re: we cannot remove the rotation entirely -- what I meant in the original comment was that we won't be able to simplify the codebase because we have to leave the code there for certain cases. It will most possibly just make it even more complicated. But I really like the idea of a rotating handler, I will play with this approach to see whether it will work out.

We will also have to agree on how much of a breaking change we (and users) could accept. Because technically both changing the default primary handler from HttpClientHandler to SocketsHttpHandler, and changing the way HandlerLifetime applies (recreating the whole chain of handlers vs just recreating the connection within the SocketsHttpHandler, leaving the chain as a technical named singleton) could break quite a lot of people, especially the second one -- because handlers from the chain could have scoped dependencies. They are not working well now (yes, #47091 😣😭) but implementing just this feature without #47091 will break scopes completely.

[CarnaViire]

Triage: Not urgent, moving to Future. The two main parts of the issue are

configure a connection lifetime for SocketsHttpHandler

-- brings the biggest value, and is already addressed by #101808

disable handler rotation

-- nice to have, not urgent plus there are existing concerns listed above (blocked by #47091)

[kevinchalet]

#101808 was locked so I can't share my feedback there.

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

FWIW, this change breaks OpenIddict - according to NuGet.org, the largest project using Microsoft.Extensions.Http.Polly and Microsoft.Extensions.Http.Resilience (to put things into perspective 😄) - as it downcasts PrimaryHandler to HttpClientHandler to be able to set properties like UseCookies:

options.HttpMessageHandlerBuilderActions.Add(builder =>
{
    if (builder.PrimaryHandler is not HttpClientHandler handler)
    {
        throw new InvalidOperationException(SR.FormatID0373(typeof(HttpClientHandler).FullName));
    }

    // Note: automatic content decompression can be enabled by constructing an HttpClient wrapping
    // a generic HttpClientHandler, a SocketsHttpHandler or a WinHttpHandler instance with the
    // AutomaticDecompression property set to the desired algorithms (e.g GZip, Deflate or Brotli).
    //
    // Unfortunately, while convenient and efficient, relying on this property has a downside:
    // setting AutomaticDecompression always overrides the Accept-Encoding header of all requests
    // to include the selected algorithms without offering a way to make this behavior opt-in.
    // Sadly, using HTTP content compression with transport security enabled has security implications
    // that could potentially lead to compression side-channel attacks if the client is used with
    // remote endpoints that reflect user-defined data and contain secret values (e.g BREACH attacks).
    //
    // Since OpenIddict itself cannot safely assume such scenarios will never happen (e.g a token request
    // will typically be sent with an authorization code that can be defined by a malicious user and can
    // potentially be reflected in the token response depending on the configuration of the remote server),
    // it is safer to disable compression by default by not sending an Accept-Encoding header while
    // still allowing encoded responses to be processed (e.g StackExchange forces content compression
    // for all the supported HTTP APIs even if no Accept-Encoding header is explicitly sent by the client).
    //
    // For these reasons, OpenIddict doesn't rely on the automatic decompression feature and uses
    // a custom event handler to deal with GZip/Deflate/Brotli-encoded responses, so that servers
    // that require using HTTP compression can be supported without having to use it for all servers.
    if (handler.SupportsAutomaticDecompression)
    {
        handler.AutomaticDecompression = DecompressionMethods.None;
    }

    // OpenIddict uses IHttpClientFactory to manage the creation of the HTTP clients and
    // their underlying HTTP message handlers, that are cached for the specified duration
    // and re-used to process multiple requests during that period. While remote APIs are
    // typically not expected to return cookies, it is in practice a very frequent case,
    // which poses a serious security issue when the cookies are shared across multiple
    // requests (which is the case when the same message handler is cached and re-used).
    //
    // To avoid that, cookies support is explicitly disabled here, for security reasons.
    handler.UseCookies = false;

    // ...
});

While designing that a while ago, I considered downcasting to other well-known handler types, but:

• They are lots of types to support if you want to cover most cases: HttpClientHandler, SocketsHttpHandler, WinHttpHandler, CFNetworkHandler, NSUrlSessionHandler, etc.
• Not all of them are public, e.g BrowserHttpHandler.

I'll probably add a HttpMessageHandlerBuilderAction that resets PrimaryHandler to a HttpClientHandler and release a new version before .NET 9.0 ships to mitigate that breaking change, but that would have been nice if you had improved that terrible design before adopting this breaking change: it's basically the same issue that has plagued the RSA base class for years before useful methods were added to the base class, eliminating the needs for downcasts.

[kevinchalet]

but that would have been nice if you had improved that terrible design before adopting this breaking change: it's basically the same issue that has plagued the RSA base class for years before useful methods were added to the base class, eliminating the needs for downcasts.

I can personally think of 3 options to make HttpClientHandler easier to compose:

• Make the *Handler implementations derive from HttpClientHandler - or a new common class derived from HttpMessageHandler - (maybe not ideal from a backcompat' perspective).
• Provide a HttpClientHandler.Wrap(HttpMessageHandler handler) static method that would return a HttpClientHandler using the specified instance as the inner handler (so the HttpClientHandler properties would mutate the inner handler).
• Introduce new interfaces that would be implemented by all the *Handler classes.

[kevinchalet]

BTW, a quick GitHub Code search reveals that other projects will be broken too for the same reasons.

E.g: https://github.com/abraxas-labs/voting-library-dotnet/blob/a1e8115f006a15a630d6632bf72206932ac8a671/src/Voting.Lib.Common/Net/CertificatePinningHandlerBuilder.cs#L45

[stephentoub]

@CarnaViire, what would you like to do here?

[CarnaViire]

TL;DR: I still need some time to reach a decision.

Thanks for bringing this up @kevinchalet. I still think that HttpClientFactory using HttpClientHandler by default is an implementation detail that shouldn't be depended on, but I do see your point -- e.g. we had to do some rather ugly casts ourselves 😞 (it doesn't break, yeah, it works as expected in all cases, but it is ugly)

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

Lines 31 to 40 in 29fe6a8

	if (builder.PrimaryHandler is HttpClientHandler httpClientHandler)
	{
	// Don't overwrite factory if one is already set.
	httpClientHandler.MeterFactory ??= _meterFactory;
	}
	else if (!OperatingSystem.IsBrowser() && builder.PrimaryHandler is SocketsHttpHandler socketsHttpHandler)
	{
	// Don't overwrite factory if one is already set.
	socketsHttpHandler.MeterFactory ??= _meterFactory;
	}

Unfortunately I guess there's no good solution in the 9.0 timeframe.

I'm inclined to keep the change, but I need some time to think and consider pros and cons. Reverting the change is fast and easy 😅 but it was introduced for a reason, as well.

I see that you have already implemented a workaround @kevinchalet. I wonder if ConfigureHttpClientDefaults could also work for you for that matter, for a cleaner configuration:

// all clients will have HttpClientHandler by default
services.ConfigureHttpClientDefaults(b =>
    b.ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler()));

or even

services.ConfigureHttpClientDefaults(b =>
    b.ConfigurePrimaryHttpMessageHandler(() =>
    {
        var handler = new HttpClientHandler() { UseCookies = false };
        if (handler.SupportsAutomaticDecompression)
        {
            handler.AutomaticDecompression = DecompressionMethods.None;
        }
        return handler;
    }));

[kevinchalet]

Thanks for bringing this up @kevinchalet.

My pleasure! Thanks for taking the time to reply.

I still think that HttpClientFactory using HttpClientHandler by default is an implementation detail that shouldn't be depended on.

I agree. Unfortunately, downcasting is pretty much required as soon as you want to tweak the properties of the handler (RSA and ECDsa suffered from the same issue). If there was a better option, we'd definitely use it 😄

it doesn't break, yeah, it works as expected in all cases, but it is ugly

"works as expected in all cases" seems a bit optimistic to me: it only works if the user doesn't replace the default primary handler (that can now be a SocketsHttpHandler or an HttpClientHandler in 9.0+) but it breaks when using a different type: you can of course no-op if the handler type is not recognized as in the snippet you shared, but it's sadly an absolute no-go for me when you need to set security-related properties such as the ones overridden by OpenIddict.

I'm inclined to keep the change, but I need some time to think and consider pros and cons. Reverting the change is fast and easy 😅 but it was introduced for a reason, as well.

It's certainly a positive change, but we really need a better way to amend the handler properties without having to manually downcast to every possible type 😅
I listed a few possible options but I'm sure there are others. That would be nice if you could think about that as such an API would be a life saver.

I see that you have already implemented a workaround @kevinchalet. I wonder if ConfigureHttpClientDefaults could also work for you for that matter, for a cleaner configuration:

No doubt it would work, but it would affect every HTTP client and not just the clients needed by the OpenIddict libraries, including user-defined clients, which is a bit meh (I don't want to force users to use HttpClientHandler for their own clients).

Thanks!