HttpClient wrapper for simple calls with optional cert

Trying to ensure each controller contains it’s own httpclient. The super user can change which endpoint is called from the site and some of those endpoints do not require a certificate, but some require their own certificate.

Want a safe call to this wrapper class (HttpClientProvider) as a static and in a static constructor for reuse. Any gotchas?

Will the httpclient still execute after not being used for a while?

Here is the Controller Call:

    public class HomeController : Controller
{
    private static readonly HttpClientProvider _clientProvider;

    static HomeController()
    {
        _clientProvider = new HttpClientProvider();
    }

    public ActionResult Index()
    {
        var certPath = HostingEnvironment.MapPath("~/App_Data/cert.pfx");
        var httpClient = _clientProvider.GetHttpClient(certPath);

        var result = httpClient.GetAsync("www.example.com").Result;

        return View();
    }
}

Here is the Provider Class:

 public class HttpClientProvider
{
    private HttpClient _client;
    private WebRequestHandler _clientHandler;

    public HttpClient GetHttpClient()
    {
        SetUpHandler();
        SetUpClient();

        return _client;
    }

    public HttpClient GetHttpClient(string certPath, string password = "")
    {
        SetUpHandler();
        SetUpClient();
        SetUpCertificate(certPath, password);

        return _client;
    }

    public HttpClient GetHttpClient(X509Certificate2 x509Cert)
    {
        SetUpHandler();

        _clientHandler.ClientCertificates.Add(x509Cert);

        SetUpClient();

        return _client;
    }

    private void SetUpHandler()
    {
        if (_clientHandler == null)
        {
            _clientHandler = new WebRequestHandler();
        }
        else
        {
            _clientHandler.ClientCertificates.Clear();
        }
    }

    private void SetUpCertificate(string certPath, string password = "")
    {
        if (certPath != null)
        {
            var absolutePath = HostingEnvironment.MapPath(certPath);

            var cert = absolutePath != null
                ? new X509Certificate2(absolutePath, password, X509KeyStorageFlags.MachineKeySet)
                : new X509Certificate2(certPath, password, X509KeyStorageFlags.MachineKeySet);

            _clientHandler.ClientCertificates.Add(cert);
        }
    }

    private void SetUpClient()
    {
        if (_client == null)
        {
            _client = new HttpClient(_clientHandler);
        }
    }
}

As is this does not work because multiple requests will concurrently operate on the same variables (instance variables and HttpClient internal state as it is being configured).

This is fairly easy to solve because there’s a nice pattern for this. Your core problem is that you need to initialize one expensive object for each instance of a certain kind of key (here: HttpClient and certificate path).

I’m modifying some existing code to ensure that only one HttpClient per key is ever created:

public static class HttpClientManager
{
    private static ConcurrentDictionary<string, Lazy<HttpClient>> _clients = 
        new ConcurrentDictionary<string, Lazy<HttpClient>>();

    public static HttpClient Get(string certificatePath)
    {
        return _clients.GetOrAdd(certificatePath, _ =>
            new Lazy<HttpClient>(CreateClient(certificatePath));
    }  

    static HttpClient CreateClient(string certificatePath)
    {
        //TODO
    }
}

You only need to fill in the TODO.

This code is thread safe because the infrastructure takes care of only invoking your factory method once per unique certificatePath. The factory will only use local state so that it is totally unconcerned with thread safety.

I’d avoid methods such as void SetUpXxx(...). These messods make use of side-effects for no reason. In particular, _clientHandler seems to be used only as a way to thread this value through the various methods being called. It is often better to avoid passing data in such covert ways. Convert that field to a method parameter and explicitly pass that value around. I’d do the same thing for _client itself. Then, you can keep the SetUpXxx methods but they become static. It will be explicit what data they consume and what they output.

This might seems like a subtle point but I found this kind of change helpful over and over again.