HttpClient scope analyzer

[mthjones]

Problem

If HttpClient is used improperly, it can lead to port exhaustion on the app server due to opening and closing ports frequently instead of reusing the ports already available. This generally happens when HttpClient is created and destroyed too frequently in a low-level function instead of being shared across multiple calls.

Analyzer

An analyzer is added to warn when an HttpClient is created within a function. This would most likely need to work on a whitelist basis to allow for proper uses, such as the creation of an HttpClient within Main.

Failure Criteria

• HttpClient is created within a non-constructor function
• HttpClient is used within the function
  • Filter out factory methods

Example

public async Task MakeRequest() {
    using( var client = new HttpClient() ) {
//         ^---- WARN: HttpClient scope may be too small which can lead to port exhaustion.
//                     Consider sharing this HttpClient across multiple requests.
        var response = await client.GetAsync( "http://example.com" );
        // do something
    }
}

Additional Resources

• https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

[omsmith]

Holding onto new HttpClient() forever is also wrong without other mitigations.

Within the LMS, the correct analyzer would be to ban it altogether and tell people to use IHttpClientFactory where we can potentially do the right thing. After that, it likely matters less whether the end-user keeps it as long-held or not (though the details of that would be dependent on how the factory wants to be consumed in ths end).

[mthjones]

Holding onto new HttpClient() forever is also wrong without other mitigations.

++. I think this is something that could be mitigated as well. (e.g. ban HttpClient field in Singleton objects or as static fields)

Within the LMS, the correct analyzer would be to ban it altogether and tell people to use IHttpClientFactory where we can potentially do the right thing. After that, it likely matters less whether the end-user keeps it as long-held or not (though the details of that would be dependent on how the factory wants to be consumed in ths end).

I agree -- that seems like the best option long-term. However, this is currently causing issues today with port exhaustion due to improper usage of the HttpClient from IHttpClientFactory (D2L, not .NET) because it doesn't do the right thing yet. If fixing that is relatively simple, then let's do that instead.