7

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());
}

}

sadhat75
  • 191
  • 4
  • 9
  • 1
    Any reason that `CallHttpClientGet` is not async? By calling `.Result` you are blocking the thread and inviting potential deadlocks. – Todd Menier Apr 21 '15 at 12:08

1 Answers1

11

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.

Todd Menier
  • 37,557
  • 17
  • 150
  • 173
  • "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? – Ohad Schneider Jul 04 '17 at 18:52
  • 2
    @OhadSchneider It's based on [Daryl Miller's advice](https://stackoverflow.com/a/22561368/62600) 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](https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/) 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. – Todd Menier Jul 16 '17 at 14:46