1
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ client should have a backing abstraction and injected into controllers. clients should be kept based on domain being accessed. For example all calls to https://somehost should be handled by the same client. That way if there needs to be any common headers set there would be no need to repeat them. \$\endgroup\$ – Nkosi Jan 13 '18 at 2:17
  • \$\begingroup\$ so i should abstract away the client and inject that abstraction? can it be singleton per controller? would a key value pair work for selecting endpoint to httpclient in abstraction? \$\endgroup\$ – asunrey Jan 13 '18 at 4:45
  • 1
    \$\begingroup\$ Take a look at this suggested answer on SO stackoverflow.com/a/48138819/5233410 \$\endgroup\$ – Nkosi Jan 13 '18 at 12:19
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the explanation. Did not know where to turn because I have little guidance and no mentors! So no need to use depency injection and inject the HttpManager class, just call it as a utility? \$\endgroup\$ – asunrey Jan 18 '18 at 1:38
  • \$\begingroup\$ @asunrey Not sure I understand, but I'd pick the simplest solution that solves the problem. No need to overuse patterns. Maybe you had an OOP mindset in your solution. OOP has it's place but it's just another pattern like all others. OOP is not suitable as a framework for everything. Hope I understood your comment correctly and this helps. \$\endgroup\$ – usr Jan 23 '18 at 12:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.