Bad example of SocketsHttpHandler.PooledConnectionLifetime usage for "typed client"

Gladskih commented 1 month ago

Type of issue

Missing information

Description

There are two issues with code example on the page

services.AddHttpClient(name)
    .ConfigurePrimaryHttpMessageHandler(() =>
    {
        return new SocketsHttpHandler()
        {
            PooledConnectionLifetime = TimeSpan.FromMinutes(2)
        };
    })
    .SetHandlerLifetime(Timeout.InfiniteTimeSpan); // Disable rotation, as it is handled by PooledConnectionLifetime

You create SocketsHttpHandler manually with the new operator. In general it means that you are becoming responsible for calling Disposable on it. So as far as we in DI registration why not use provider argument to get an instance? But then the type itself should be registered in DI and it seems it needs to be transient to allow the factory pool it. And by the way, why do you believe 2 minutes is good? DNS defines TTL, so probably you have to check its record (which is not so simple either because OS provided API just resolves to IP). But even if TTL is elapsed and DNS record is changed to another IP, why shall we shut down a live connection? I know that old idea about DNS based load balancing, but if a server evicted from a cluster and don't want to serve clients it can simply close all the connections. Summarizing I would suggest something like this

    services.AddTransient(_ => new SocketsHttpHandler 
    {
        // Even DNS change is not the reason to close the connection until the server did
        PooledConnectionLifetime = Timeout.InfiniteTimeSpan 
    })
    .AddHttpClient<IPayloadHasher<Uri>, PayloadHasher>()
    .ConfigurePrimaryHttpMessageHandler(provider => provider.GetRequiredService<SocketsHttpHandler>())
    .SetHandlerLifetime(Timeout.InfiniteTimeSpan); // Disable rotation, as it is handled by PooledConnectionLifetime

But I am not sure in all implementation details of these leaky abstractions and maybe ConfigurePrimaryHttpMessageHandler is not needed then at all (if it's the default "bastard-injected" implementation)

Page URL

https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory#using-ihttpclientfactory-together-with-socketshttphandler

Content source URL

https://github.com/dotnet/docs/blob/main/docs/core/extensions/httpclient-factory.md

Document Version Independent Id

33db41be-42f9-bb94-65fe-0cf7d1c64f12

Article author

@IEvangelist

Metadata

ID: 46af0b96-2d12-9cac-2f8f-15eb49d0292d
Service: dotnet-fundamentals

IEvangelist commented 1 month ago

I'm not entirely certain, and it's best to pull in the SMEs from the networking team. Let's ask @CarnaViire for a bit of guidance here.

CarnaViire commented 1 month ago

You create SocketsHttpHandler manually with the new operator. In general it means that you are becoming responsible for calling Disposable on it.

It does not. The code isn't "creating" it "manually", it is providing a callback -- the factory method -- to the HttpClientFactory. And the factory will be the one executing it -- and then "owning" the resulting instances. The factory is the one responsible for creation, pooling/caching, and disposal of the handlers. It is "playing a part" of DI, if you wish. From the factory perspective, it really doesn't matter whether the handler is registered in DI or not, as long as a fresh new instance is returned each time.

(If you'd created an instance outside of the callback, and then made the callback return this existing instance, then technically you'd will be the one responsible, but that approach is erroneous as it breaks a "contract" of a factory method)

why do you believe 2 minutes is good

I agree that there's no universally good default, and that's exactly why SocketsHttpHandler does NOT have a default PooledCollectionLifetime value. That also means that SocketsHttpHandler at the moment is not "ready" to be used "out of the box", without additional set up.

On the other hand, historically, HttpClientFactory decided to be opinionated instead, and chose to have at least some (a bit arbitrary) default, that would work "out of the box" in most cases. Thus, HandlerLifetime was chosen to default to 2 minutes. For all these years, it really seems it is working fine in most cases.

But then again, I don't "believe 2 minutes is good". 2 minutes are here ONLY because it's a default HandlerLifetime value. But it could easily be 5 minutes instead. It doesn't matter that much. The goal of an example is not to give you an IDEAL solution that would work for EVERYONE. The goal of an example is ILLUSTRATE the idea. 2 minutes (as well as 5 minutes 😄) would just work better than SocketsHttpHandler's infinity.

(You are not naming your classes "MyService" just because it's how they're named in the examples, are you? 😄)

DNS defines TTL, so probably you have to check its record (which is not so simple either because OS provided API just resolves to IP)

I am well aware of that.

If you'd like to discuss and/or propose changes to SocketsHttpHandler behavior, including how connection pooling works, feel free to explore existing issues or open a new one in the dotnet/runtime repo.

E.g. here's a couple of DNS-related ones https://github.com/dotnet/runtime/issues/19443 https://github.com/dotnet/runtime/issues/60390

Gladskih commented 1 month ago

2 minutes (as well as 5 minutes 😄) would just work better than SocketsHttpHandler's infinity.

That's a very bold statement. If you don't have a DNS based load balancing with a malfunctioning backend (which doesn't care to close connection after eviction from a cluster) you will just break connection for no reason if I understand it correctly.

You are not naming your classes naming your classes "MyService" just because it's how they're named in the examples

I do not, that's why I am here, but a lot of developer do such things. An example from official documentation ends up being copy pasted all over stackoverflow.com, articles and commercial code bases. And any vague point will grow yet another cargo cult.

And when even authors of a framework do not know an ideal solution in their own platform it's even more important to declare all the caveats in the documentation.

karelz commented 1 month ago

That's a very bold statement.

It is actually capturing the reality of current behavior of SocketsHttpHandler - Infinity value in current implementation is a BAD setting, which breaks customers and makes their lives miserable.

If you don't have a DNS based load balancing with a malfunctioning backend (which doesn't care to close connection after eviction from a cluster) you will just break connection for no reason if I understand it correctly.

Yes, that is correct. That is the BEST our current SocketsHttpHandler implementation can do. We would like it to be better, but sadly, it is what it is now.

And when even authors of a framework do not know an ideal solution in their own platform it's even more important to declare all the caveats in the documentation.

The ideal solution of the platform/framework (the CURRENT implementation of the platform/framework) is the one in the docs - so I think we are in agreement.

dotnet / docs