Static HttpClient thread safe on ASP.net HttpRequest

Question

We are creating a wrapper for HttpClient. As we are going to follow performance optimization guidance from https://github.com/mspnp/performance-optimization. We want to avoid anti-pattern - Improper instantiation mentioned in that document. I referred this guidance to my team to use static HttpClient. The feedback I have got is on thread-safety. Each request has a header containing user claim. Since I have a static HttpClient, will it be thread-safe? If we have multiple requests hitting the code (for example GET) at the same time, will it be a race condition to set header? We have implementation as below.

public class HttpClientHelper{
private static readonly HttpClient _HttpClient;
static HttpClientHelper() {
 HttpClient = new HttpClient();
 HttpClient.Timeout = TimeSpan.FromMinutes(SOME_CONFIG_VALUE);
}
public async Task<HttpResponseMessage> CallHttpClientPostAsync(string requestUri, HttpContent requestBody)
{
 AddHttpRequestHeader(httpClient);
 var response = await httpClient.PostAsync(requestUri, requestBody); //Potential thread synchronization issue???
 return response;
}
public HttpResponseMessage CallHttpClientGet(string requestUri)
{
 AddHttpRequestHeader(httpClient);
 var response = httpClient.GetAsync(requestUri).Result; //Potential thread synchronization issue???
 return response;
}
private void AddHttpRequestHeader(HttpClient client)
{
 string HeaderName = "CorrelationId";
 client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(Properties.Settings.Default.HttpClientAuthHeaderScheme, GetTokenFromClaims()); //Race condition???
 if (client.DefaultRequestHeaders.Contains(HeaderName))
 client.DefaultRequestHeaders.Remove(HeaderName);
 client.DefaultRequestHeaders.Add(HeaderName, Trace.CorrelationManager.ActivityId.ToString());
}

}

• Todd Menier: Any reason that CallHttpClientGet is not async? By calling .Result you are blocking the thread and inviting potential deadlocks.

Answer 1

Your team is correct, this is far from thread safe. Consider this scenario:

• Thread A sets CorrelationId header to "foo".
• Thread B sets CorrelationId header to "bar".
• Thread A sends request, which contains thread B's CorrelationId.

A better approach would be for your CallXXX methods to create new HttpRequestMessage objects, and set the header on those, and use HttpClient.SendAsync to make the call.

Keep in mind also that re-using HttpClient instances is only beneficial if you're making multiple calls to the same host.

• Ohad Schneider: "Keep in mind also that re-using HttpClient instances is only beneficial if you're making multiple calls to the same host" - do you have a reference for this?
• Todd Menier: @OhadSchneider It's based on Daryl Miller's advice to use one instance "for each distinct API that you connect to". Reason being that performance benefits (not having to open a new connection, etc) are only relevant per host, as are certain HttpClient properties like DefatultHeaders. However, the now-famous socket problem could change my advice a little. Can Windows reclaim a socket in TIME_WAIT for use with a different host? I'm not sure. I posted the question on that article.
• AnujGeek: Won't there be a race condition now when setting the headers for the HttpRequestMessage object? Edit: If you create a new local instance of HttpRequestMessage, each call will refer to the instance specific to that call only, so won't cause a race condition.