Creating a thread-safe version of a c# statistics service

Question

I have an API that people are calling and I have a database containing statistics of the number of requests. All API requests are made by a user in a company. There's a row in the database per user per company per hour. Example:

| CompanyId | UserId| Date             | Requests |
|-----------|-------|------------------|----------|
| 1         | 100   | 2020-01-30 14:00 | 4527     |
| 1         | 100   | 2020-01-30 15:00 | 43       |
| 2         | 201   | 2020-01-30 14:00 | 161      |

To avoid having to make a database call on every request, I've developed a service class in C# maintaining an in-memory representation of the statistics stored in a database:

public class StatisticsService
{
    private readonly IDatabase database;
    private readonly Dictionary<string, CompanyStats> statsByCompany;
    private DateTime lastTick = DateTime.MinValue;

    public StatisticsService(IDatabase database)
    {
        this.database = database;
        this.statsByCompany = new Dictionary<string, CompanyStats>();
    }

    private class CompanyStats
    {
        public CompanyStats(List<UserStats> userStats)
        {
            UserStats = userStats;
        }

        public List<UserStats> UserStats { get; set; }
    }

    private class UserStats
    {
        public UserStats(string userId, int requests, DateTime hour)
        {
            UserId = userId;
            Requests = requests;
            Hour = hour;
            Updated = DateTime.MinValue;
        }

        public string UserId { get; set; }
        public int Requests { get; set; }
        public DateTime Hour { get; set; }
        public DateTime Updated { get; set; }
    }
}

Every time someone calls the API, I'm calling an increment method on the StatisticsService:

public void Increment(string companyId, string userId)
{
    var utcNow = DateTime.UtcNow;
    EnsureCompanyLoaded(companyId, utcNow);

    var currentHour = new DateTime(utcNow.Year, utcNow.Month, utcNow.Day, utcNow.Hour, 0, 0);

    var stats = statsByCompany[companyId];
    var userStats = stats.UserStats.FirstOrDefault(ls => ls.UserId == userId && ls.Hour == currentHour);
    if (userStats == null)
    {
        var userStatsToAdd = new UserStats(userId, 1, currentHour);
        userStatsToAdd.Updated = utcNow;
        stats.UserStats.Add(userStatsToAdd);
    }
    else
    {
        userStats.Requests++;
        userStats.Updated = utcNow;
    }
}

The method loads the company into the cache if not already there (will publish EnsureCompanyLoaded in a bit). It then checks if there is a UserStats object for this hour for the user and company. If not it creates it and set Requests to 1. If other requests have already been made for this user, company, and current hour, it increments the number of requests by 1.

EnsureCompanyLoaded as promised:

private void EnsureCompanyLoaded(string companyId, DateTime utcNow)
{
    if (statsByCompany.ContainsKey(companyId)) return;
    var currentHour = new DateTime(utcNow.Year, utcNow.Month, utcNow.Day, utcNow.Hour, 0, 0); ;

    var userStats = new List<UserStats>();
    userStats.AddRange(database.GetAllFromThisMonth(companyId));

    statsByCompany[companyId] = new CompanyStats(userStats);
}

The details behind loading the data from the database are hidden away behind the GetAllFromThisMonth method and not important to my question.

Finally, I have a timer that stores any updated results to the database every 5 minutes or when the process shuts down:

public void Tick(object state)
{
    var utcNow = DateTime.UtcNow;
    var currentHour = new DateTime(utcNow.Year, utcNow.Month, utcNow.Day, utcNow.Hour, 0, 0);

    foreach (var companyId in statsByCompany.Keys)
    {
        var usersToUpdate = statsByCompany[companyId].UserStats.Where(ls => ls.Updated > lastTick);
        foreach (var userStats in usersToUpdate)
        {
            database.Save(GenerateSomeEntity(userStats.Requests));
            userStats.Updated = DateTime.MinValue;
        }
    }

    // If we moved into new month since last tick, clear entire cache
    if (lastTick.Month != utcNow.Month)
    {
        statsByCompany.Clear();
    }

    lastTick = utcNow;
}

I've done some single-threaded testing of the code and the concept seem to work out as expected. Now I want to migrate this to be thread-safe but cannot seem to figure out how to implement it the best way. I've looked at ConcurrentDictionary which might be needed. The main problem isn't on the dictionary methods, though. If two threads call Increment simultaneously, they could both end up in the EnsureCompanyLoaded method. I know of the concepts of lock in C#, but I'm afraid to just lock on every invocation and slow down performance that way.

Anyone needed something similar and have some good pointers in which direction I could go?

Answer 1

When keeping counters in memory like this you have two options:

1. Keep in memory the actual historic value of the counter
2. Keep in memory only the differential increment of the counter

I have used both approaches and I've found the second to be simpler, faster and safer. So my suggestion is to stop loading UserStats from the database, and just increment the in-memory counter starting from 0. Then every 5 minutes call a stored procedure that inserts or updates the related database record accordingly (while zero-ing the in-memory value). This way you'll eliminate the race conditions at the loading phase, and you'll ensure that every call to Increment will be consistently fast.

For thread-safety you can use either a normal Dictionary with a lock, or a ConcurrentDictionary without lock. The first option is more flexible, and the second more efficient. If you choose Dictionary+lock, use the lock only for protecting the internal state of the Dictionary. Don't lock while updating the database. Before updating each counter take the current value from the dictionary and remove the entry in an atomic operation, and then issue the database command while other threads will be able to recreate the entry again if needed. The ConcurrentDictionary class contains a TryRemove method that can be used to achieve this goal without locking:

public bool TryRemove (TKey key, out TValue value);

It also contains a ToArray method that returns a snapshot of the entries in the dictionary. At first glance it seems that the ConcurrentDictionary suits your needs, so you could use it as a basis of your implementation and see how it goes.

Answer 2

In general, to make your code thread-safe:

• Use concurrent collections, such as ConcurrentDictionary

• Make sure to understand concepts such as lock statement, Monitor.Wait and Mintor.PulseAll in tutorials. Locks can be slow if IO operations (such as disk write/read) it being locked on, but for something in RAM it is not necessary to worrry about. If you have really some lengthy operation such as IO or http requests, consider using ConcurrentQueue and learn about the consumer-producer pattern to process work in queues by many workers (example)

• You can also try Redis server to cache database without need to design something from zero.

• You can also make your service singleton, and update database only after value changes. For reading value, you have already stored it in your service.

Answer 3

To avoid having to make a database call on every request, I've developed a service class in C# maintaining an in-memory representation of the statistics stored in a database:

If you want to avoid Update race conditions, you should stop doing exactly that.

Databases by design, by purpose prevent simple update race conditions. This is a simple counting-up operation. A single DML statement. Implicity protected by transactions, journaling and locks. Indeed that is why calling them a lot is costly.

You are fighting the concurrency already there, by adding that service. You are also moving a DB job outside of the DB. And Moving DB jobs outside of the DB, is just going to cause issues.

If your worry is speed:

1. Please read the Speed Rant.
2. Maybe a Dsitributed Database Design is the droid you are looking for? They had a massive surge in popularity since Mobile Devices have proliferated, both for speed and reliability reasons.