WebDavClient icon indicating copy to clipboard operation
WebDavClient copied to clipboard

IHttpClientFactory

Open grahamehorner opened this issue 6 years ago • 8 comments

Please consider adding support for IHttpClientFactory, HttpPolicies

grahamehorner avatar Jul 30 '18 12:07 grahamehorner

Hi @grahamehorner,

I haven't thoroughly researched IHttpClientFactory, so it's not clear to me what benefits it brings. Could you explain in more detail what use cases you have and how IHttpClientFactory can be integrated with WebDavClient, please?

WebDavClient has a constructor that accepts a pre-configured instance of HttpClient, so technically you can use IHttpClientFactory to create HttpClient and pass it to WebDavClient.

var httpclient = _httpClientFactory.CreateClient();
var webdav = new WebDavClient(httpclient);

Is it enough for your use cases?

skazantsev avatar Aug 07 '18 21:08 skazantsev

It's important for me to see substantial benefits because this feature doesn't come for free:

IHttpClientFactory lives in Microsoft.Extensions.Http and requires .NET Standard 2.0. It's not a problem to migrate to .NET Standard 2.0 since I was planning to do it anyway. But I was hoping to keep supporting .NET Standard 1.1 and add .NET Standard 2.0 just as another target. In this case, I have to exclude IHttpClientFactory from .NET Standard 1.1, which also not a big problem but comes with additional work and maintenance cost, which I would love to avoid.

skazantsev avatar Aug 07 '18 21:08 skazantsev

@skazantsev I have used IHttpFactory with .NET standard 1.6 and above although the ASP.NET core team haven't built the nuget pacakage a re-compile of the source targeting 1.6 works well, it may be worth looking 1.6 given the guidance around release/support cycle of the ASP.NET core libraries.

grahamehorner avatar Aug 08 '18 13:08 grahamehorner

@skazantsev or event using some of the lessons learned by the ASP.NET core team and IHttpClientFactory to implement a .NET Standard 1.1 version

grahamehorner avatar Aug 08 '18 13:08 grahamehorner

@skazantsev the HttpClientFactory helps prevent wasteful creation of multiple HttpClient objects that are threadsafe that have the same http host endpoint; it also can handle transient errors, retry, backoff with/without jitter and/or caching of requests.

grahamehorner avatar Aug 08 '18 13:08 grahamehorner

Before introducing IHttpClientFactory the best practice for using HttpClient was to create a single instance per application. Since WebDavClient just wraps HttpClient the best practice to use it without IHttpClientFactory is to also instantiate it once per application.

In order to use WebDavClient with IHttpClientFactory just instantiate it with HttpClient created by the factory.

var httpclient = _httpClientFactory.CreateClient();
var webdav = new WebDavClient(httpclient);

Does it work for your use cases?

skazantsev avatar Aug 08 '18 19:08 skazantsev

@skazantsev ,

Before introducing IHttpClientFactory the best practice for using HttpClient was to create a single instance per application.

Here's the problem. WebDavClient disposes an instance of HttpClient passed via constructor. For example, someone wants to reuse the same HttpClient instance, but it won't be possible after disposing WebDavClient instance. I am talking about the .NET Standard 1.1 project where IHttpClientFactory is not used.

I suggest adding an input parameter (e.g., bool disposeHttpClient) that signals if WebDavClient needs to dispose the specified HttpClient. To keep backward compatibility this parameter can have true as default value.

The same technique is widely used when Stream argument is passed to class constructors. For example, the StreamReader constructor has leaveOpen parameter for this purpose. https://docs.microsoft.com/en-us/dotnet/api/system.io.streamreader.-ctor?view=netframework-4.6#System_IO_StreamReader__ctor_System_IO_Stream_System_Text_Encoding_System_Boolean_System_Int32_System_Boolean_ image

Andrei-Sheshka avatar Dec 10 '20 20:12 Andrei-Sheshka

@Andrei-Sheshka,

For example, someone wants to reuse the same HttpClient instance, but it won't be possible after disposing WebDavClient instance.

I wonder if you're talking about re-using the same HttpClient instance across multiple WebDavClient instances or in WebDavClient + some classes making calls to different APIs?

In the first scenario, I would create a single instance of WebDavClient for the app lifetime (similar to HttpClient), which typically means using the Singletone lifetime in a DI-container or a static property. This way you almost never need to dispose it yourself.

In the second scenario, I would avoid sharing HttpClient between WebDavClient and other classes. It's better to create HttpClient per API you need to talk to (each HttpClient is still a singletone).

Does this solve your problem or am I missing something?

skazantsev avatar Dec 17 '20 14:12 skazantsev