Do not create a new HttpClient for every Transmission

[cwe1ss]

https://github.com/Microsoft/ApplicationInsights-dotnet/blob/develop/src/Core/Managed/Shared/Channel/Transmission.cs#L62 creates a new HttpClient instance for every transmission.

See e.g. https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ for why this is wrong (it creates too many sockets and they stay open for too long).

[SergeyKanzhelev]

Thank you for reporting the issue. Feel free to send a PR.

[cwe1ss]

Not sure if I have time for it. Would you agree to this approach:

• Make the HttpClient instance a static variable on Transmission
• Each transmission gets a timeout value but HttpClient only has a global Timeout-property. According to the HttpClient source, a possible workaround would be:
• • Set the global Timeout to "Infinite"
• • Use the SendAsync overload that takes a CancellationToken and pass a token that expires after the Transmission-timeout value

Does this need additional tests in TransmissionTest.cs?

[SergeyKanzhelev]

I'd say move it away from the transmission class and make HttpClient be owned by the sender. This way the lifecycle of a single HttpClient is much easier to control. And there will be no problems of one httpclient shared by multiple senders and attempts to send data the same time.

[cwe1ss]

I agree to that. However there's conditional #if behavior with HttpClient and the old WebRequest in there, so this would probably require a bigger refactoring and I definitely don't want to do that right now.

[SergeyKanzhelev]

@cwe1ss with the approach you proposed you'll need to synchronize access to static variable. Do you have an idea how to make it efficient?

It may not be a huge refactoring if you'll keep some wrapper class in Sender that will be used by transmissions.

[kogir]

HttpClient is thread safe if used correctly. You don't need to synchronize access to it, just its construction.

Safe methods:

CancelPendingRequests
DeleteAsync
GetAsync
GetByteArrayAsync
GetStreamAsync
GetStringAsync
PostAsync
PutAsync
SendAsync

See https://msdn.microsoft.com/en-us/library/system.net.http.httpclient(v=vs.110).aspx#Anchor_5

[SergeyKanzhelev]

Thanks @kogir! It makes sense now to go with the simpler fix and just reuse a static HttpClient.

[cijothomas]

Closing via #857