-
-
Notifications
You must be signed in to change notification settings - Fork 735
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
Comments
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? |
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:
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)...
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:
Then I made each interface implement it:
Then I created the following unit test:
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:
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:
Note in the above: IBoringCrudWithClientApi<User, string> implements IBoringCrudApi<User, string> and IHttpClientProvider, 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:
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. |
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. |
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>())
The text was updated successfully, but these errors were encountered: