U4-11570 - Do not use Webclient

Created by laurent lequenne 13 Aug 2018, 07:52:20 Updated by Shannon Deminick 20 Aug 2018, 14:56:02

Tags: Up For Grabs PR

And certainly do not create a new instance of it at each usage. Either use a unique static HttpClient, or use underlying objects which are more performant.

Code from KeepAlive.aspx

    using (var wc = new HttpClient()) 
    { 
                      var request = new HttpRequestMessage(HttpMethod.Get, url); 
                     var result = await wc.SendAsync(request, token); 
    } 

Comments

Sebastiaan Janssen 13 Aug 2018, 14:06:18

I've marked as "Up for grabs" so that you or someone else coming along could create a pull request for it.


Sebastiaan Janssen 17 Aug 2018, 11:59:18

Fixed in PR: https://github.com/umbraco/Umbraco-CMS/pull/2860


Nicholas Westby 17 Aug 2018, 16:32:30

Is there some problem with the performance of WebClient? I've not heard of that before, and would be very interested to know more, as I use WebClient sometimes.


laurent lequenne 17 Aug 2018, 18:04:05

@Knickerbocker Webclient is not designed to be used on a server application, you should preferrably use the base implementation HttpWebRequest. Since NET 4.5, there is the HttpClient which is more optimal for asynchronous handling, but it is not recommended to create a new instance of if at each call.

Some links ; https://blogs.msdn.microsoft.com/henrikn/2012/02/16/httpclient-is-here/

https://social.msdn.microsoft.com/Forums/vstudio/en-US/2ce80a71-1ced-4bcd-adb4-88eef6e6a42d/httpclient-vs-httpwebrequest?forum=wcf


laurent lequenne 17 Aug 2018, 18:09:45

The PR make a static HttpClient on the KeepAlive.aspx, but if you look into the code base, there are a few uses of the HttpClient, so I'm not sure that's the right implementation, as we could end up with multiple static HttpClient instances, which is useless memory/resources consumption.

So it could be better to have a global HttpClient, or some HttpClient that can be injected on demand… More likely on request scope :-)


Nicholas Westby 17 Aug 2018, 18:15:27

I took a look at those links and it seems like HttpClient has some interesting capabilities, though I didn't see much in regards to performance.

Maybe the main performance gain is that you only need a single instance? Or is there some other performance benefit to use HttpClient rather than WebClient?


Steve 17 Aug 2018, 18:38:59

@Knickerbocker https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ It's not a performance issue. When you dispose the HttpClient it will still hold the connection open for 240 seconds and might cause a socket exception because too many connections are open at once. Using an existing instance will reuse sockets.


Nicholas Westby 17 Aug 2018, 19:09:45

Thanks, @SteveV. That makes sense. By using HttpClient, we can reuse a single instance (in the very least, it's easier to do so). And by using a single instance, you can prevent a full connection pool.


laurent lequenne 17 Aug 2018, 20:19:27

@Knickerbocker The WebClient is not safe at all on an asp.net application. We had some usage of it on a heavy loaded site, and it fails randomly. Probably related to a number of limited concurrent connections


Shannon Deminick 20 Aug 2018, 14:56:02

Just be aware that a static instance of HttpClient comes with it's own drawbacks as well: http://byterot.blogspot.com/2016/07/singleton-httpclient-dns.html. Whether this could affect Umbraco sites is anyones guess.

We don't use HttpClient as a critical part in our code, it's not used very often like a REST client would use it. That said if you guys want to create an internal singleton in the Umbraco.Web project to expose an HttpClient then by all means go for it. We'll have to replace it in v8 with a container registration but that's ok.

This is all why .NET Core has the new HttpClientFactory (see https://www.stevejgordon.co.uk/introduction-to-httpclientfactory-aspnetcore) to solve these problems.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.12.1

Sprint:

Story Points:

Cycle: