4
\$\begingroup\$

Basic Background

I have an ASP.NET Core MVC application that uses Client (e.g., "customer") information for every HTTP request. The collection of Clients, as well as their information, rarely changes and is stored in a database. If it does change and some subsequent requests still use the old information, or even if the application throws an exception because of the change, that is OK.

Goal

I would like my ClientService class to cache the Client information to remove making a database call on every request. However, I believe that making this class a singleton in the DI container means that it needs to be thread-safe. (If I am wrong about this, I am still interested in whether the code below would be.) The thread-safety of this code and its conformance to best practices for thread-safety is my primary goal, although performance considerations would also be appreciated.

Code

Below is the ClientService class, which is the primary class that I am working on. I have also included the Clone method of the Client class since it is referenced. ClientService is added to the DI container as a singleton in Startup.

One pretty significant question...

Are locks even needed here? This is my first time using them.

First time Code Review post- please advise of any improvements that this site would prefer. Thanks in advance for your time reading!

public class ClientService
{
    private readonly IServiceProvider _serviceProvider;
    private readonly string _key;

    public ClientService(
        IKeyProvider keyProvider,
        IServiceProvider serviceProvider
        )
    {
        _serviceProvider = serviceProvider;
        _key = keyProvider.GetKey();
    }

    private readonly object _cachedClientsLock = new object();
    private Dictionary<int, Client> _cachedClients = null;

    public async Task<Client> GetClient(int clientId)
    {
        await EnsureCache();

        lock (_cachedClientsLock)
        {
            return _cachedClients[clientId].Clone();
        }
    }

    public async Task<Client> GetClientByCode(string code)
    {
        await EnsureCache();

        lock (_cachedClientsLock)
        {
            foreach (Client client in _cachedClients.Values)
                if (client.Code == code)
                    return client.Clone();
        }

        return null;
    }

    private async Task EnsureCache()
    {
        bool clientsAreCached;
        lock (_cachedClientsLock)
        {
            clientsAreCached = _cachedClients != null;
        }

        if (!clientsAreCached)
        {
            Dictionary<int, Client> clients = await GetClients();
            lock (_cachedClientsLock)
            {
                _cachedClients = clients;
            }
        }
    }

    private async Task<Dictionary<int, Client>> GetClients()
    {
        using (IServiceScope scope = _serviceProvider.CreateScope())
        {
            ConnectionStringProvider connectionStringProvider = scope.ServiceProvider.GetRequiredService<ConnectionStringProvider>();
            string connectionString = await connectionStringProvider.GetConnectionString();

            // SQL execution using SqlCommand and SqlDataReader

            Dictionary<int, Client> results = new Dictionary<int, Client>();

            // SqlDataReader populates the dictionary
            // _key is used here

            return results;
        }
    }
}

public class Client
{
    // Properties

    public Client Clone()
    {
        return new Client()
        {
            Id = this.Id,
            Code = this.Code,
            // etc. for remaining properties, which are all strings
        };
    }
}

public class Startup
{
    public void ConfigureServices(IServiceCollection services)
    {
        services.AddSingleton<IClientService, ClientService>();

        // other services and configuration
    }
}
```
\$\endgroup\$
3
  • 1
    \$\begingroup\$ "SQL execution using SqlCommand and SqlDataReader" Why? Use an ORM like Dapper or Entity Framework. And if you use Entity Framework there's caching built-in, so you don't even need to use singletons etc. \$\endgroup\$
    – BCdotWEB
    Dec 24 '21 at 8:47
  • \$\begingroup\$ @BCdotWEB An ORM may provide a good solution (EF for the task at hand, for the reason you mentioned), but I'm interested in the multi-threaded issues regardless so that I can take the principles and use them for other situations as well that may not involve a persistent data source. As for Dapper, I tend to write my mappings instead... I've used it before and have it in some projects (usually ones with teams), but it's mostly just my personal preference that drives the decision to not use it most times. \$\endgroup\$
    – elmer007
    Dec 27 '21 at 19:30
  • \$\begingroup\$ @BCdotWEB Thanks for the suggestion about using EF's caching functionality. I need to read more about its pros/cons, since adding it to the project primarily for caching the Client info would be a significant decision. \$\endgroup\$
    – elmer007
    Dec 27 '21 at 19:32
3
\$\begingroup\$

The locks on the read are not needed since the cache is only created once and the rest is just readonly access. If wanting locks since most of these calls is read access then the ReaderWriterLockSlim class would be an option to not serilize the reading of the dictionary.

Another option that would IMO simplify the code is to use the Lazy class. We can combine the Lazy with Task to make sure we only ever make one Task that all the threads share. Once the Task is complete it will just return the same result when awaited again. I changed the code to return IReadOnlyDictionary as it tells others coming after you the dictionary should only be read and not updated. Also GetClient I used TryGetValue as to not throw keynotfound. Even if wanting to throw an exception when the key isn't found that generic error isn't helpful when debugging. Throwing a custom ClientNotFoundException or at least change the message so it's clear what the problem is.

A downside of this approach is if the GetClients throws that exception would be cached for all calls. If wanting to retry if exception I would suggest adding the Polly nuget package and using it in the GetClients code.

Example code of using Lazy<Task<>>

public class ClientService
{
    private readonly IServiceProvider _serviceProvider;
    private readonly string _key;
    private readonly Lazy<Task<IReadOnlyDictionary<int, Client>>> _cachedClients;

    public ClientService(
        IKeyProvider keyProvider,
        IServiceProvider serviceProvider
        )
    {
        _serviceProvider = serviceProvider;
        _key = keyProvider.GetKey();
        _cachedClients = new Lazy<Task<IReadOnlyDictionary<int, Client>>>(GetClients);
    }

    public async Task<Client> GetClient(int clientId)
    {
        var cachedClients = await _cachedClients.Value;
        if (cachedClients.TryGetValue(clientId, out Client client))
        {
            return client.Clone();
        }
        return null;
    }

    public async Task<Client> GetClientByCode(string code)
    {
        var cachedClients = await _cachedClients.Value;
        return cachedClients.Values.FirstOrDefault(client => client.Code == code);
    }
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for these comments! There are several aspects I'm reading into now (and will be trying out implementation-wise soon), but definitely some improvements. I really like the note about changing to readonly to better describe the functionality. My initial follow-up would be to make sure I'm understanding correctly when you say the "locks on the read are not needed" due to the read only nature; when looking at the ReaderWriterLockSlim class, it appears to lock for reads. Is non-locked reading safe due to the assumption that writing won't be taking place concurrently, only other reads? \$\endgroup\$
    – elmer007
    Dec 27 '21 at 19:43
  • 1
    \$\begingroup\$ Since you are always calling the ensure cache which will lock to update there is no need to lock on the reads. I was just saying if you still wanted to lock, which I don’t know why. Use the readerwriterslim as its less overhead than the full blown lock \$\endgroup\$ Dec 28 '21 at 0:12

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.