(Go: >> BACK << -|- >> HOME <<)

Skip to content
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

feature: Reuse HttpClient in Refit extension method #918

Open
rlanhellas opened this issue Apr 29, 2020 · 3 comments
Open

feature: Reuse HttpClient in Refit extension method #918

rlanhellas opened this issue Apr 29, 2020 · 3 comments

Comments

@rlanhellas
Copy link

Is your feature request related to a problem? Please describe.
In some cases, we have a pre-configured HttpClient and I would like to use it in "services.AddRefitClient()" method. Today the method addRefitClient() always create a new HttpClient, an undesired behavior in some cases.

Describe the solution you'd like
I think a good solution should be to give the option to pass a HttpClient name in RefitSettings, and instead add a new HttpClient (like code bellow, just reuse an existent).
return services.AddHttpClient(UniqueName.ForType<T>())

@search4sound
Copy link

I agree with this idea (shouldn't have multiple HttpClients with the same base address), but it is not trivial - even if AddRefitClient called AddHttpClient(name), it would still create a new HttpClient for each Refit type T as long as AddTypedClient is still being called. This may be better suited as a feature request to modify the Microsoft.Extensions.DependencyInjection (specifically HttpClientBuilderExtensions and possibly HttpClientMappingRegistry). It is possible for the Refit library to have its own functionality similar to AddTypedClient, but it wouldn't be able to leverage internal classes like HttpClientMappingRegistry. I have an idea of an alternate version of AddRefitClient that has its own HttpClient registry logic rather than calling AddTypedClient. Is there interest in a PR for this?

@search4sound
Copy link
search4sound commented Aug 2, 2022

I implemented a solution and have a PR ready. I want to make sure my solution is tied to a feature request issue description, so I did a full write-up:

Is your feature request related to a problem? Please describe.

It is generally best practice to reuse HttpClient where feasible, such as when different API calls can use the same HttpClient with the same base address. While all the methods for a given Refit-enabled interface will use the same HttpClient, some methods may be better suited to be defined in different interfaces, following the Interface Segregation Principle that is the "I" in SOLID. At the least, a high-level segregation between OAuth methods (AuthGrantFromCode, AuthGrantFromRefreshToken, AuthGrantFromClientCreds) and other API data access and CRUD methods would make sense. If I have two interfaces, ISomeSiteAuth, and ISomeSiteData, and they both use the same base address, then if I register them like this:

builder.Services.AddRefitClient<ISomeSiteAuth>().ConfigureHttpClient(httpClient => httpClient.BaseAddress = new Uri("https://foo"));
builder.Services.AddRefitClient<ISomeSiteData>().ConfigureHttpClient(httpClient => httpClient.BaseAddress = new Uri("https://foo"));

Then the underlying HttpClients will be different instances, because the underlying call to AddTypedClient does this for each interface (inside internal static method Microsoft.Extensions.DependencyInjection.AddTypedClientCore)...

builder.Services.AddTransient<TClient>(s =>
{
    IHttpClientFactory httpClientFactory = s.GetRequiredService<IHttpClientFactory>();
    HttpClient httpClient = httpClientFactory.CreateClient(builder.Name);

    return factory(httpClient, s);
});

I was able to prove this by adding to each interface a property get of type HttpClient and property name Client. For reusability, I put this property in a new interface:

public interface IHttpClientProvider
{
    HttpClient Client { get; }
}

Then I made each interface implement it:

public interface ISomeSiteAuth : IHttpClientProvider { /* ISomeSiteAuth details omitted */ }
public interface ISomeSiteData : IHttpClientProvider { /* ISomeSiteData details omitted */ }

Then I created the following unit test:

[Fact]
public void RefitClientsGenericWithoutSettingsGetTheirOwnHttpClients()
{
    var services = new ServiceCollection();

    services.AddRefitClient<ISomeSiteAuth>();
    services.AddRefitClient<ISomeSiteData>();

    var authApi = services.BuildServiceProvider().GetRequiredService<ISomeSiteAuth>();
    var dataApi = services.BuildServiceProvider().GetRequiredService<ISomeSiteData>();
    var sameClient = ReferenceEquals(authApi.Client, dataApi.Client);

    Assert.False(sameClient);
}

I found this test passes - the references of the Client property of the authApi and the dataApi are different. I ran this in an actual full fledge application and got the same result. Note I have a unit test for the non-generic AddRefit method as well and get the same result.

Describe the solution you'd like

I would like to have the ability to optionally make these two interfaces use the same HttpClient instance. When calling AddRefit, have information in the parameters (new properties in RefitSettings would be good) which the AddRefit method could use to determine whether to behave as it currently does or provide new behavior where the HttpClient used for one Refit interface type could be used for another Refit interface type.

Describe alternatives you've considered

I looked into whether this behavior could be changed with the existing Refit library and whether Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.AddTypedClient could be called in such a way that reusing the HttpClient could be possible, but I could not achieve this - there is too much hidden inside internal methods. I suppose a PR on HttpClientBuilderExtensions could be requested, but this seems to me to be more a limitation of how Refit works, where you only have interfaces and rely greatly on the underlying behavior.

Describe suggestions on how to achieve the feature

I suggest that in AddRefitClient where we are currently always calling AddTypedClient, we conditionally call it - call it if there are no RefitSettings or if the RefitSettings has no property set to request the HttpClient to be reused. However if RefitSettings does have such a property set (not null or empty), then the code should keep track of registered HttpClients and provide an existing one of it matches the criteria in settings, otherwise provide a new HttpClient.

One way to implement this is to add two properties to the RefitSettings class:

public string? HttpClientName { get; set; }
public Uri? HttpClientBaseAddress { get; set; }

We may only need one of these properties to establish which HtpClient a Refit interface should use, however I think having these two provides flexibility - you may want more than one HttpClient per BaseAddress (one use case I've encountered was where we wanted one HttpClient for long polling calls and another for regular fast returning calls, may be other use cases applicable to Refit).

I would have the code track HttpClients in a dictionary with key being a string (set to settings.HttpClientName.) and value being the HttpClient When creating an HttpClient, I would set the BaseAddress to the settings.HttpClientBaseAddress, this way it does not need to be set following an AddRefitClient call and things simply behave as expected based on the settings.

I implemented a solution and created unit tests for this functionality. The first test ensures multiple interfaces share the same HttpClient when they share the same string value for HttpClientName in RefitSettings:

[Fact]
public void RefitClientsGenericWithSameSettingsHttpClientNameShareTheSameHttpClient()
{
    var services = new ServiceCollection();

    var refitSettings = new RefitSettings
    {
        HttpClientName = "ClientName1", HttpClientBaseAddress = new Uri("https://localhost:5001"),
    };
    services.AddTransient(_ => refitSettings);

    services.AddRefitClient<IBoringCrudWithClientApi<User, string>>(refitSettings);
    services.AddRefitClient<IBoringOtherCrudWithClientApi<Role, string>>(refitSettings);

    var boringApi = services.BuildServiceProvider().GetRequiredService<IBoringCrudWithClientApi<User, string>>();
    var boringOtherApi = services.BuildServiceProvider().GetRequiredService<IBoringOtherCrudWithClientApi<Role, string>>();
    var sameClient = ReferenceEquals(boringApi.Client, boringOtherApi.Client);

    Assert.True(sameClient);

    // Also confirm the client has the same base address as the settings base address.
    Assert.Equal(boringApi.Client.BaseAddress?.AbsoluteUri, refitSettings.HttpClientBaseAddress?.AbsoluteUri);
}

Note in the above: IBoringCrudWithClientApi<User, string> implements IBoringCrudApi<User, string> and IHttpClientProvider,
and IBoringOtherCrudWithClientApi<User, string> implements IBoringOtherCrudApi<User, string> and IHttpClientProvider, thus providing access to each api implementation's Client property.

Both the RefitClientsGenericWithoutSettingsGetTheirOwnHttpClients test and the RefitClientsGenericWithSameSettingsHttpClientNameShareTheSameHttpClient test pass with my implementation.

I also wrote a test for the case where multiple interfaces do not the same HttpClient when they gave different string values for HttpClientName in RefitSettings:

[Fact]
public void RefitClientsGenericWithDifferentSettingsHttpClientNameGetTheirOwnHttpClients()
{
    var services = new ServiceCollection();

    var refitSettings = new RefitSettings
    {
        HttpClientName = "ClientName1", HttpClientBaseAddress = new Uri("https://foo:5001"),
    };
    services.AddTransient(_ => refitSettings);

    services.AddRefitClient<IBoringCrudWithClientApi<User, string>>(refitSettings);

    var refitSettings2 = new RefitSettings
    {
        HttpClientName = "ClientName2", HttpClientBaseAddress = new Uri("https://bar:5002"),
    };
    services.AddTransient(_ => refitSettings2);

    services.AddRefitClient<IBoringOtherCrudWithClientApi<Role, string>>(refitSettings2);

    var boringApi = services.BuildServiceProvider().GetRequiredService<IBoringCrudWithClientApi<User, string>>();
    var boringOtherApi = services.BuildServiceProvider().GetRequiredService<IBoringOtherCrudWithClientApi<Role, string>>();
    var sameClient = ReferenceEquals(boringApi.Client, boringOtherApi.Client);

    Assert.False(sameClient);

    // Also confirm each client base address equals the corresponding settings base address.
    Assert.Equal(boringApi.Client.BaseAddress?.AbsoluteUri, refitSettings.HttpClientBaseAddress?.AbsoluteUri);
    Assert.Equal(boringOtherApi.Client.BaseAddress?.AbsoluteUri, refitSettings2.HttpClientBaseAddress?.AbsoluteUri);
}

I have a corresponding set of unit tests for the non-generic version of the AddRefit method as well.

Additional context

My solution is ready to be submitted as a PR if this functionality is deemed acceptable.

@search4sound
Copy link

After researching this more, I think the advantages of having separate HttpClient for each refit interface type outweigh the disadvantages. The issue is the HttpMessageHandlers must not be shared or cached. I think it is best to just set the HttpMessageHandlerFactory property of RefitSettings to a factory that provides hanlders for each HttpClient created on behalf of a refit interface type. I realize there are some who for unit tests are mocking HttpClient and want the refit clients to use the mocked HttpClient, though there are challenges with mocking HttpClient (does not implement an interface or inherit from an abstract class, only from a concrete class that implements IDisposable). It should be sufficient to mock the handler, that's where the http calls are made.

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