7
\$\begingroup\$

I have created an ASP.NET Core singleton service class to act as an HttpClient provider after reading that reusing an instance of HttpClient is encouraged. However, I also want to be sure that the BaseAddress property is kept up-to-date with the value specified in the configuration so I added a dependency on IOptionsMonitor so that any time the configuration is updated, the BaseAddress property is re-set.

HttpClientProvider

public class MachineDatabaseHttpClientProvider
{
    public HttpClient Client
    {
        get
        {
            try
            {
                readWriteLock.EnterReadLock();
                return client;
            }
            finally
            {
                readWriteLock.ExitReadLock();
            }
        }
    }

    private readonly ReaderWriterLockSlim readWriteLock = new ReaderWriterLockSlim();
    private readonly HttpClient client;

    public MachineDatabaseHttpClientProvider(IOptionsMonitor<ApplicationConfiguration> applicationConfigurationMonitor)
    {
        client = new HttpClient();
        client.BaseAddress = new Uri(applicationConfigurationMonitor.CurrentValue.MachineDatabase.BaseUrl);
        client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

        applicationConfigurationMonitor.OnChange(c => UpdateClientBaseAddress(c.MachineDatabase.BaseUrl));
    }

    private void UpdateClientBaseAddress(string baseUrl)
    {
        try
        {
            readWriteLock.EnterWriteLock();
            client.BaseAddress = new Uri(baseUrl);
        }
        finally
        {
            readWriteLock.ExitWriteLock();
        }
    }
}

ASP.NET Core Startup Class

public class Startup
{
    public Startup(IHostingEnvironment env) { ... }

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddMvc().AddTypedRouting();
        services.Configure<ApplicationConfiguration>(Configuration.GetSection(ApplicationConfiguration.ConfigurationSectionKey));
        services.AddSingleton<MachineDatabaseHttpClientProvider>();
    }
}

Is the above code more complex than it needs to be? Is my use of ReaderWriterLockSlim sufficient to achieve thread-safety? Reading this answer on HttpClient thread-safety indicates that none of the properties of HttpClient are thread-safe. Should I offer Get and Post methods as a wrapper around the HttpClient so that I can control all access to the class?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ How can a public class with a public constructor be a true singleton? Or are you saying you are using like a singleton, as in there is only one instance of this class in your application? \$\endgroup\$ Mar 31, 2017 at 16:53
  • \$\begingroup\$ @GregBurghardt I assume that the class is registered as a singleton with the IoC container. \$\endgroup\$ Mar 31, 2017 at 17:44
  • \$\begingroup\$ @GregBurghardt yes, it is as mentioned by Igor, it is registered as a singleton via the out of the box IoC container provided by ASP.NET Core. \$\endgroup\$
    – M Michal
    Mar 31, 2017 at 19:34

2 Answers 2

2
\$\begingroup\$

Alternative proposal

I think, I'd approach it differently, but I may miss some important details.

As long as the configuration change does not happen too frequently OR client construction is cheap, we can actually create a new instance of the HttpClient on every configuration change event, rather than trying to keep a single object in a good state. IMO, this approach is cleaner in the functional programming sense: the Client property now becomes a pure function in a way -- no messing with state of the object.

Now, you probably still want to keep the same HttpClient instance for as long as the config does not change -- to avoid an unnecessary object construction. Your HttpClientProvider can then work as a client factory and client "cache" in a way. This class will be responsible for providing either an existing properly configured HttpClient instance, or generating a new one since configuration has changed.

As a side note, I assume you use an IoC container for injecting HttpClientProvider into the consumers. The provider is probably registered as a Singleton with the IoC container, right?

Nitpick

There's no need to immediately create a client instance on MachineDatabaseHttpClientProvider construction. This can be deferred until the Client instance is being accessed by the consumer (e.g. Lazy<HttpClient> may work).

P. S.

I know that my answer is twisting the problem definition. Hopefully, this is not an issue (especially if there are no other requirements that are forcing you to have a single instance of an HttpClient throughout the entire application lifetime).

--------------- UPDATE 1 ---------------

I am not a guru of multi-threading but I don't think you should have any issue in most of the cases even with a thread-unsafe code here. I have different concerns though (explained below).

Simplicity

If you lock around the _currentHttpClient object [obviously, not lock (_currentHttpClient) but rather lock (_currentHttpClientLock)], you will guarantee that NO unnecessary client is created "ever". Otherwise, you may have an extra object created n times per configuration change when there're n threads asking for a client "simultaneously".

Warnings/Pitfalls

Non-locking approach is not an issue only as long as it's okay to swap the "hot" client. I.e. the one used by a consumer for performing multiple client calls within a single body of work. It also depends a lot on how the consumer is using the HttpClient. Here are three different scenarios with their own problems.

Option 1

Here's a consumer that does not hold an HttpClient reference, but keeps retrieving the client from provider for EACH external call:

class ConsumingController
{
    private MachineDatabaseHttpClientProvider _provider;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _provider = provider;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        ComplexObject complexObject;

        // Moment 1
        complexObject = _myOwnCopyOfClient.Client.GetObject("base" + id);

        // Moment 2 
        complexObject.Details = _myOwnCopyOfClient.Client.GetObject("datails-for-" + id);

        return complexObject;
    }
}

If the configuration changes between the moments 1 and 2 (say endpoint changes), the two requests will target different databases and it may result in funny results. And I mean, funny in a bad way.

In a good case, you'll get an HTTP 404 for the second request, which is safe. All the consuming application needs to do is repeat the request...

In a worse case, there could be "detail" objects in the second DB that match the id from the first DB. This will result in construction of an object that presents data from two incompatible sources.

You application should be able to handle this situation.

Option 2

Another consumer design will simply hold the reference to the HttpClient instance:

class ConsumingController
{
    private HttpClient _myOwnCopyOfClient;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _myOwnCopyOfClient = provider.Client;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        ComplexObject complexObject;
        complexObject = _myOwnCopyOfClient.GetObject("base" + id);
        complexObject.Details = _myOwnCopyOfClient.GetObject("datails-for-" + id);
        return complexObject;
    }
}

Notice that if the consumer persists the reference to an HttpClient, there's NO effective WAY for the Provider to update the configuration of that instance.

Option 3

This may be the best option...

class ConsumingController
{
    private MachineDatabaseHttpClientProvider _provider;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _provider = provider;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        // This client instance is bound to the method execution scope...
        var requestScopeClient = _myOwnCopyOfClient.Client;

        ComplexObject complexObject;

        // Moment 1
        complexObject = requestScopeClient.GetObject("base" + id);

        // Moment 2 
        complexObject.Details = requestScopeClient.GetObject("datails-for-" + id);

        return complexObject;
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Igor, your solution will work great because you assumed correctly, the single initial instance does not need to be present for the entire runtime of the application. With your suggestion, locking is still required, correct? \$\endgroup\$
    – M Michal
    Mar 31, 2017 at 19:50
  • \$\begingroup\$ Upon further reading, it looks like Lazy<T> is thread-safe. However, since I'll be updating the new field when the configuration changes, I believe that I will still require locks for that portion. \$\endgroup\$
    – M Michal
    Mar 31, 2017 at 19:56
  • \$\begingroup\$ @MMichal see my update. I hope I am not diverging from the original question. \$\endgroup\$ Mar 31, 2017 at 20:48
  • \$\begingroup\$ Also, you only need to lock when you rewrite the httpClient reference. Absolutely no need to lock on read. \$\endgroup\$ Mar 31, 2017 at 21:30
2
\$\begingroup\$

There is an abstraction IOptionsSnapshot<T> that will automatically have any config updates when injected, so you don't need IOptionsMonitor<T>. I believe it is scoped per request, so changes to the config will not affect in-flight requests (you may want to double check this).

To deal with updating the HttpClient, you can write a small, thread safe HttpClientFactory that will always return the same client for a particular base URL. This would be registered as a singleton, and could be reused for all the clients in your app.

public sealed class HttpClientFactory : IDisposable    // Register as singleton
{
    private readonly ConcurrentDictionary<Uri, HttpClient> _httpClients;

    public HttpClientFactory()
    {
        _httpClients = new ConcurrentDictionary<Uri, HttpClient>();
    }

    public HttpClient Create(Uri baseAddress)
    {
        return _httpClients.GetOrAdd(baseAddress,
            b => new HttpClient {BaseAddress = b});
    }

    public void Dispose()
    {
        foreach (var httpClient in _httpClients.Values)
        {
            httpClient.Dispose();
        }
    }
}

You can use this along with IOptionsSnapshot<T> to configure clients as needed.

public class MachineDbClientFactory    // Register as transient or request scoped
{
    private readonly HttpClientFactory _httpClientFactory;
    private readonly IOptionsSnapshot<AppSettings> _settings;

    public MachineDbClientFactory(HttpClientFactory httpClientFactory,
        IOptionsSnapshot<AppSettings> settings)
    {
        _httpClientFactory = httpClientFactory;
        _settings = settings;
    }

    public MachineDbClient Create()
    {
        var baseAddress = _settings.Value.FooServiceAddress;
        var httpClient = _httpClientFactory.Create(baseAddress);
        return new MachineDbClient(httpClient);
    }
}

Note that I have assumed you are wrapping HttpClient inside a MachineDbClient here, however you could choose to make MachineDbClientFactory behave similarly to your MachineDatabaseHttpClientProvider and return an HttpClient if that is what your application needs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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