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

Guidance on integrating with HttpClientFactory #654

Open
stijnherreman opened this Issue Jan 14, 2019 · 12 comments

Comments

Projects
None yet
6 participants
@stijnherreman
Copy link

stijnherreman commented Jan 14, 2019

Currently I register some dependencies like this:

var fooHttpClient = new HttpClient();
fooHttpClient.BaseAddress = new Uri($"{this.appSettings.FooUri}/");
container.Register<Foo.IClient>(
	() => new Foo.Client(fooHttpClient),
	Lifestyle.Scoped
	);

I want to move to HttpClientFactory.

  • I can register a Named Client with IServiceCollection.AddHttpClient(...) but then I'd have to resolve the named client when configuring the SimpleInjector container. (I haven't tried yet if this is actually possible.)
  • I can register a Typed Client with IServiceCollection.AddHttpClient(...) but then SimpleInjector is not involved at all.

Do you have any guidance on this subject?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jan 14, 2019

Moving to the new HttpClientFactory model is good, considering all the pitfalls that exist with the problematic design of HttpClient.

Unfortunately, the injection of typed clients (i.e. using AddHttpClient) is very tightly coupled with the built-in container, and still quite error prone.

Instead, I'd advise injecting IHttpClientFactory into your HttpClient's consumers and use its CreateClient method to resolve the required client, much like is described here.

Here's an example:

public sealed class HttpGithubClient : IGithubClient
{
    private readonly IHttpClientFactory clientFactory;
    private readonly HttpGithubClientSettings settings;
    
    public HttpGithubClient(
        IHttpClientFactory clientFactory,
        HttpGithubClientSettings settings)
    {
        this.clientFactory = clientFactory;
        this.settings = settings;
    }

    public async Task<GitHubBranch[]> GetBranches()
    {
        var request = new HttpRequestMessage(
            HttpMethod.Get,
            this.settings.GithubRootUrl + "/repos/aspnet/docs/branches");
        request.Headers.Add("Accept", "application/vnd.github.v3+json");
        request.Headers.Add("User-Agent", this.settings.UserAgent);

        using (var client = this.clientFactory.CreateClient())
        {
            var response = await client.SendAsync(request);

            if (response.IsSuccessStatusCode)
            {
                return await response.Content
                    .ReadAsAsync<IEnumerable<GitHubBranch>>()
                    .ToArray();
            }
            else
            {
                throw new HttpException("...");
            }
        }
    }
}

This example is an altered version of the one in the MS doc, with the following changes:

  • The component is stateless, prevent your components from having state whenever possible
  • As an example, the component uses a HttpGithubClientSettings configuration object. This allows allows required configuration values to be supplied by the Composition Root, if required. The configuration object is specific to this particular component.
  • The created HttpClient is now wrapped in a using statement. HttpClient instances should still be disposed.
  • The method throws an exception on failure, instead of returning error codes using the boolean property (really bad idea).
@stijnherreman

This comment has been minimized.

Copy link
Author

stijnherreman commented Jan 14, 2019

Thank you for the info. I'll have a look and see what's possible, these clients are generated by https://github.com/RSuter/NSwag but there's a lot of configuration possible.

@rexcfnghk

This comment has been minimized.

Copy link

rexcfnghk commented Jan 15, 2019

Shouldn't HttpGitHubClient avoid depending on a framework abstraction and instead define its own IHttpClientManager interface instead?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jan 15, 2019

Shouldn't HttpGitHubClient avoid depending on a framework abstraction and instead define its own IHttpClientManager interface instead?

Not when you consider the HttpGitHubClient to be the adapter implementation. The IGitHubClient is in that case the application-specific abstraction.

@rexcfnghk

This comment has been minimized.

Copy link

rexcfnghk commented Jan 15, 2019

Shouldn't HttpGitHubClient avoid depending on a framework abstraction and instead define its own IHttpClientManager interface instead?

Not when you consider the HttpGitHubClient to be the adapter implementation. The IGitHubClient is in that case the application-specific abstraction.

I see. That makes sense! Thanks!

@ralphhendriks

This comment has been minimized.

Copy link

ralphhendriks commented Jan 28, 2019

@dotnetjunkie I am puzzled by this part of your answer

The created HttpClient is now wrapped in a using statement. HttpClient instances should still be disposed.

I think that last statement is an incorrect advice. The Mircrosoft docs quite clearly describe what is wrong with disposing HttpClient:

As a first issue, while this class is disposable, using it with the using statement is not the best choice because even when you dispose HttpClient object, the underlying socket is not immediately released and can cause a serious issue named ‘sockets exhaustion’.

Wouldn't explicitly disposing the HttpClient generated by the clientFactory the lifetime management solution offered by IHttpClientFactory?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jan 28, 2019

@ ralphhendriks,

That statement by Microsoft is wrong. It's not the problem that HttpClient is disposed, but it is the problem that its underlying HttpMessageHandler is disposed and not reused. The HttpClientFactory fixes exactly this problem. When you let HttpClientFactory create new HttpClient instances for you, it will ensure that HttpClient will not dispose it underlying HttpMessageHandler when the HttpClient is disposed. This allows the HttpMessageHandler to be pooled and reused.

You can understand more about this issue by reading this, this, and this.

@ralphhendriks

This comment has been minimized.

Copy link

ralphhendriks commented Jan 29, 2019

@dotnetjunkie that makes sense. Thank you for the detailed explanation.

I think this is an issue many developers will struggle with. Would it be an idea to add some hints to the docs? If you like I could do a first attempt, but maybe this is in better hands with someone more intimately familiar with the subject matter 😊

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jan 29, 2019

Hi @ralphhendriks,

It will probably be just enough to add a link to this discussion from Other Technologies section of the integration page. I just added that link.

@ziongh

This comment has been minimized.

Copy link

ziongh commented Feb 11, 2019

@dotnetjunkie your explanation about stateless components make total sense.

So we would register the IHttpClientFactory as a Singleton. However the implementation that is used by microsoft services.AddHttpClient() is internal (DefaultHttpClientFactory) and depends on IServiceProvider and other internal classes.

You would recomend using Cross-Wiring for this interface or developing our own implementation?

Just to be shure, if I choose to develop my own implementation, I would need to track the lifetime of HttpMessageHandler and some race conditions.

And considering the use o Polly for a resilient HttpClient, you would recomend adding this logic on the implementation of IHttpClientFactory.CreateHttpClient?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Feb 11, 2019

I recommend using the DefaultHttpClientFactory and let it be injected using Simple Injector's auto-wiring facilities, exactly because it is an internal and complex component. Do not implement your own. That would be unproductive.

I'm unfamiliar with Polly and how it compares to the new IHttpClientFactory, so I can't comment on that.

@voroninp

This comment has been minimized.

Copy link

voroninp commented Feb 20, 2019

@dotnetjunkie Do I understand correctly that typed clients are not supported by SimpleInjector?

I add typed client services.AddHttpClient<EmailClient>(), but get an exception:

System.InvalidOperationException
ActivationException: The constructor of type EmailClient contains the parameter with name 'httpClient' and type HttpClient that is not registered. Please ensure HttpClient is registered, or change the constructor of ConnectEmailClient.

Update:
Ok. My bad. It turned out that typed client is the type which accepts HttpClient, so I register not the mapping between HttpClient and type, but the type itself.
However, if this typed client depends on other services besides HttpClient, and they are registered with SimpleInjector, container verification still fails. When I move registration to services everything works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment