Using Refit with ASP.Net Core 2.1's HttpClientFactory

[thecontrarycat]

The ASP.Net Core team have created a new way of working with HttpClient: the HttpClientFactory https://github.com/aspnet/HttpClientFactory

The idea behind HttpClientFactory is that you no longer need to worry about keeping singleton HttpClient instances around. Instead, you can get as many instances of HttpClient as you like from the factory, and the underlying HttpClientHandler will have their lifecycles managed internally. This ensures that the issues with HttpClient that added the singleton requirement such as socket exhaustion are solved, while the issues with using a singleton (such as failing to respond to DNS changes due to long-lived connections) are also handled.

Currently, to use HttpClientFactory with Refit you can simply new up an instance of your service with RestService.For<T>(HttpClient). However, this can lead to a performance reduction due to the way methods are cached (in a dictionary on the generated service class).

To make this work more smoothly, I propose moving the method caching out of the generated service class and adding it as a decorator around the RequestBuilderImplementation. This then makes the IRequestBuilder separate from the generated class, enabling us to provide a new overload to RestService.For<T>(HttpClient, IRequestBuilder), where the IRequestBuilder can be created by the caller (using RequestBuilder.For<T>() and kept around as a singleton.

I have the PR ready to go if you would like this feature.

[thecontrarycat]

Working commit: thecontrarycat@bf8b0f9

[bennor]

I think this is a good idea in theory, but do you have any measurements for the performance improvement?

[thecontrarycat]

All I've done is moved the existing caching to a different place so that people who aren't using a singleton HttpClient can benefit from it in the same way that those with a singleton HttpClient currently do.

I presume the existing caching was put there for a reason :)

[bennor]

I'm not sure when the caching was added or why. My only real concern is that we have an outstanding bug relating to concurrency (#449), and I'm not sure if that has anything to do with the existing caching. (I haven't had time to look at it yet - it may not even exist in the latest version.)

[bennor]

Other than a few minor nitpicks, the working commit looks good. I'd be happy to take the PR. Thoughts, @onovotny?

[clairernovotny]

Certainly happy to take a PR to utilize IHttpClientFactory

[thecontrarycat]

#494

[clairernovotny]

Implemented, thanks!