Skip to content

Disposing HttpClient in GraphQLHttpClient #329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
OleksandrTaranSecurrency opened this issue Mar 1, 2021 · 6 comments
Closed

Disposing HttpClient in GraphQLHttpClient #329

OleksandrTaranSecurrency opened this issue Mar 1, 2021 · 6 comments

Comments

@OleksandrTaranSecurrency

I wonder if there is any good reason to call Dispose on the HttpClient in the GraphQLHttpClient.

There are several articles in the internet that describe why it's bad. Like this one https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

@rose-a
Copy link
Collaborator

rose-a commented Mar 2, 2021

Good question... is there any good reason to dispose of GraphQLHttpClient within a running application?

It's propably more a thing of "cleaning up after itself" (a object should clean up all the resources it grabbed when it's disposed of).
The case where this theory currently fails is when a HttpClient is passed into GraphQLHttpClient using the constructor.

@OleksandrTaranSecurrency
Copy link
Author

OleksandrTaranSecurrency commented Mar 3, 2021

The case where this theory currently fails is when a HttpClient is passed into GraphQLHttpClient using the constructor.

This is actually the reason why I posted this question.

Theoretically you can check if HttpClient was instantiated internally or passed as constructor dependency and then either dispose it or do nothing.

We did run into socket exhaustion and the only place where dispose is called on HttpClient is GraphQLHttpClient.

@rose-a
Copy link
Collaborator

rose-a commented Mar 3, 2021

Why do you dispose of GraphQLHttpClient?

@OleksandrTaranSecurrency
Copy link
Author

Well I think it's absolutely logical to call dispose on the objects that implement IDisposible, however It's not me, it's DI container.

builder.Services.AddScoped<IGraphQLClient>(s => new GraphQLHttpClient(finP2POptions.GraphQlUrl, new NewtonsoftJsonSerializer()));

Since GraphQLHttpClient is IDisposable it will be disposed when a web request is done.

Alternatively we could try a singleton but according to the article that I posted above we might get DNS issues. It's highly unlikely but still possible.

I think it still makes sense to dispose HttpClient only when it was created inside GraphQLHttpClient, otherwise(if it's passed as a constructor dependency) it should be the responsibility of the calling code to decide when the httpclient should be disposed.

@rose-a
Copy link
Collaborator

rose-a commented Apr 2, 2021

Well I think it's absolutely logical to call dispose on the objects that implement IDisposible

I totally support that, but there is that strange behaviour of HttpClient which makes it hard to handle...

I think it still makes sense to dispose HttpClient only when it was created inside GraphQLHttpClient

Correct, will be adressed...

But the way you configure GraphQLHttpClient in your DI container it actually creates a new HttpClient since you don't pass one in the constructor...

@rose-a rose-a added the bug Something isn't working label Apr 2, 2021
@rose-a rose-a removed the bug Something isn't working label Apr 2, 2021
@rose-a
Copy link
Collaborator

rose-a commented Apr 2, 2021

The disposing behavoir has been fixed in v3.2.3.

Please make sure you configure your DI container to inject HttpClient into GraphQLHttpClient.

@rose-a rose-a closed this as completed Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants