Socket Exhaustion: IHttpClientFactory should be used instead of HttpClient()

[kondelik]

You should not use new HttpClient() in GraphQLHttpClient constructor, because there is known problem with new HttpClient() socket exhaustion under heavy server load.

Instead, you should inject 'IHttpClientFactory' and call (for example) var client = HttpClientFactory.CreateClient(nameof(GraphQLHttpClient))

I think i can (at least try to) do PR for you, but i fear this is breaking change, because the GraphQLHttpClient is not resolved from DI but created by hand (at least README.md show this)

Workaround is to supply HttpClient resolved from IHttpClientFactory by yourself:

(i am writing this from top of my head, maybe there is typo or two 😄 )

private readonly IHttpClientFactory _httpClientFactory;

ctor(IHttpClientFactory httpClientFactory){
  _httpClientFactory= httpClientFactory;
}

SendSomeGraphQL(string query ... )
{
  using var httpClient = _httpClientFactory.CreateClient(nameof(GraphQLHttpClient));
  var graphQLClient = 
    new GraphQLHttpClient(
      new GraphQLHttpClientOptions { EndPoint = new Uri("https://api.example.com/graphql") },
      new NewtonsoftJsonSerializer(), 
      httpClient
  );
 ...
}

Further reading:

https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

[kondelik]

Or, is my "workaround" prefered way of doing it?

(lets talk 😄 )

then it would be nice to add more constructors to support it, like

public GraphQLHttpClient(string endPoint, IGraphQLWebsocketJsonSerializer serializer, HttpClient client);
public GraphQLHttpClient(Action<GraphQLHttpClientOptions> configure, IGraphQLWebsocketJsonSerializer serializer, HttpClient client);

or even

public GraphQLHttpClient(string endPoint, IGraphQLWebsocketJsonSerializer serializer, IHttpClientFactory clientFactory);
public GraphQLHttpClient(Action<GraphQLHttpClientOptions> configure, IGraphQLWebsocketJsonSerializer serializer, IHttpClientFactory clientFactory);

That should not break anything.

[rose-a]

the GraphQLHttpClient is not resolved from DI but created by hand (at least README.md show this)

Of course you can let the DI container of your choice resolve GraphQLHttpClient. It's not in the README because this should showcase the usage of the GraphQL client, not some DI container.

You should not use new HttpClient() in GraphQLHttpClient constructor, because there is known problem with new HttpClient() socket exhaustion under heavy server load.

GraphQLHttpClient is not meant to be registered as "transient" and re-instantiated for each usage. I recommend creating a long lived instance (i.e. singleton) and reuse that for your requests. This way it won't cause socket exhaustion. It's also a client library, so no unexpected huge number of concurrent connections through it.

it would be nice to add more constructors to support it

This library targets netstandard2.0;net461, where IHttpClientFactory is not yet available.

[kondelik]

GraphQLHttpClient is not meant to be registered as "transient" and re-instantiated for each usage.

Ok, good to know! Maybe word or two somewhere in readme about this? (I am sorry I cant do this because of my engrish)

I recommend creating a long lived instance (i.e. singleton) and reuse that for your requests.

Or maybe instance which live (by default) 2 minutes, like the one from HttpClientFactory, which is what I am talking about 😉

I see you are talking about GraphQLHttpClient and I am about underlying HttpClient. I think the difference is pretty much Subscriptions. Mine would die every 2 minutes, your will just work indefinitely.

Am I right? Am I missing something?

In my use case i dont care about Subsciptions, I use GraphQL.Client for inter-micro-services communication and I was using it at first as readme show - initializing it for each use, currently with "Workaround" I posted earlier and overriden override HttpRequestMessage ToHttpRequestMessage(). Mainly because url of "other" microservice (the one i am sending request to) can change every time other-service-container get scaled/restarted/whatever (thats job for service discovery).

So, I think I can handle my problems with current implementation & I can see reasoning behind "only one ctor take HttpClient" - because you are supposed to have long-living GraphQLHttpClient (becouse of Subsciptions (?) & thats the thing I somehow missed)

[sonicmouse]

It should be up to the developer how the HttpClient gets used. A lot of assumptions are being made by forcing the HttpClient to be disposed. The GraphQL library doesn't own the client, it shouldn't delegate when it gets disposed.

A simple work-around that will not create any breaking changes would be to add an optional parameter to this constructor:

public GraphQLHttpClient(
    GraphQLHttpClientOptions options,
    IGraphQLWebsocketJsonSerializer serializer,
    HttpClient httpClient,
    bool disposeHttpClient = true);

Then set the read-only flag to whatever it's set to.