C# Thread-safe singleton service for caching data used during lifetime of ASP.NET Core MVC application

Question

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
    }
}
```

Answer 1

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