How to ensure thread safe Web API base controller method

Question

I have a web API service that will accept many concurrent requests by different clients accessing two SQL Server databases, all the controllers (around 65) have a BaseApiController, find below the common method.

protected async Task<IEnumerable<TObject>> GetSyncData<TObject>(
            Guid clientId, 
            string clientSyncTable,
            Func<Task<IEnumerable<TObject>>> initialDataFunc,
            Func<ISyncMetadataClient, Task<IEnumerable<TObject>>> sinceLastSyncDataFunc,
            Func<ISyncMetadataClient, Task<bool>> insertClientSyncMetadata,
            Func<IEnumerable<ISyncObject>, IEnumerable<TSyncMetadata>> getSyncMetadataFromSyncObjects,
            Func<IEnumerable<TObject>, Task<IEnumerable<TObject>>> getExistingObjectsWithSyncMetadata) where TObject : ISyncObject
        {
            // Get the client sync metadata.
            ISyncMetadataClient syncMetadataClient =
                await SyncDataService.ClientSyncMetadata.GetFirstAsync(new { ClientId = clientId, SyncTable = clientSyncTable })
                    .ConfigureAwait(false);
            // No metadata for this client and this resource. Use a lock or a better thread safe practice?
            if (syncMetadataClient == null)
            {
                // Client first time syncing now, return initial data.
                IEnumerable<TObject> data = await initialDataFunc().ConfigureAwait(false);
                if (data.Any())
                {
                    await CheckForMetadataMissingAndAppendAny(getSyncMetadataFromSyncObjects, getExistingObjectsWithSyncMetadata, data)
                        .ConfigureAwait(false);
                    // Here I cought exception trying to insert duplicate key. 
                    await insertClientSyncMetadata(new SyncMetadataClient
                    {
                        SyncMetadataClientId = Guid.NewGuid(),
                        ClientId = clientId,
                        LastSyncUtcDateTime = DateTime.UtcNow,
                        SyncTable = clientSyncTable
                    }).ConfigureAwait(false);
                }
                return data;
            }

            // We have ClientSyncMetadata return all data since LastSyncDateTime.
            IEnumerable<TObject> sinceLastSyncData = await sinceLastSyncDataFunc(syncMetadataClient).ConfigureAwait(false);
            return sinceLastSyncData;
        }

private async Task CheckForMetadataMissingAndAppendAny<TObject>(
            Func<IEnumerable<ISyncObject>, IEnumerable<TSyncMetadata>> getSyncMetadataFromSyncObjects, 
            Func<IEnumerable<TObject>, Task<IEnumerable<TObject>>> getExistingObjectsWithSyncMetadata, 
            IEnumerable<TObject> data)
            where TObject : ISyncObject
        {
            // Get the records that have metadata
            IEnumerable<TObject> existingDataWithSyncMetadata = await getExistingObjectsWithSyncMetadata(data).ConfigureAwait(false);
            // Select the records that has no metadata
            IEnumerable<TObject> dataWithNoMetadata;
            if (!existingDataWithSyncMetadata.Any())
            {
                dataWithNoMetadata = data.Where(p => p.CorrelationId == Guid.Empty).ToList();
            }
            else
            {
                dataWithNoMetadata = data.Where(p => !existingDataWithSyncMetadata.Any(item => p.CorrelationId == item.CorrelationId)).ToList();
            }
            // Unit of work transaction to insert all metadata in the database
            IEnumerable<TSyncMetadata> syncMetadataItems = getSyncMetadataFromSyncObjects(dataWithNoMetadata.Select(p => p as ISyncObject)).ToList();
            if (syncMetadataItems.Any())
            {
                TransactSyncMetadataCommand.AddRange(syncMetadataItems);
                await TransactSyncMetadataCommand.ExecuteAsync().ConfigureAwait(false);
            }
        }

The exception message:

Cannot insert duplicate key row in object 'dbo.SyncMetadataClient' with unique index 'SyncMetadataClient_ix00'. The duplicate key value is (70db3459-32ad-4557-9fac-405b82a5a349, Users).

The above message means that if (syncMetadataClient == null) is processed by one or more threads while another was inserting the record in the database.

I am posting the transaction class I'm using, I was thinking if I should thread safe that part and not the BaseApiController, you thoughts?

public async Task<TransactionResult> ExecuteAsync()
        {
            bool anyToInsert = DataToInsert.Any();
            bool anyToUpdate = DataToUpdate.Any();
            bool anyToDelete = DataToDelete.Any();
            TransactionResult result = new TransactionResult();

            if (anyToInsert ||
                anyToUpdate ||
                anyToDelete)
            {
                using (Connection)
                using (IDbTransaction transaction = Connection.BeginTransaction())
                {
                    try
                    {
                        // Add, Update and Delete stuff...

                        transaction.Commit();
                    }
                    catch (Exception ex)
                    {
                        transaction.Rollback();
                        result.Error = ex;
                        return result;
                    }
                    finally
                    {
                        DataToInsert.Clear();
                        DataToUpdate.Clear();
                        DataToDelete.Clear();
                    }
                }
            }
            return result;
        }

Any suggestions of a good practice to ensure thread safety or should I just put a lock around the resource access code and move on?

Answer 1

I believe you could benefit from the use of some kind of "named" lock, e.g. a different lock for every ClientId + SyncTable combination (or just a single ClientId).

This will help you handle concurrency in a more flexible way than just locking on the same object for any client request.

[Update] As stated in the comments, it is not allowed to use await inside a lock statement, thus I have updated the answer with an approach that uses SemaphoreSlim instead.

You could implement this approach using a ConcurrentDictionary:

private static ConcurrentDictionary<string, SemaphoreSlim> Locks = new ConcurrentDictionary<string, SemaphoreSlim>();

And inside your method:

var lockKey = clientId + clientSyncTable;
var sem = Locks.GetOrAdd(lockKey, x => new SemaphoreSlim(1));

await sem.WaitAsync();
try {
    ISyncMetadataClient syncMetadataClient =
                await SyncDataService.ClientSyncMetadata.GetFirstAsync(
                    new {
                        ClientId = clientId,
                        SyncTable = clientSyncTable
                    }).ConfigureAwait(false);

    if (syncMetadataClient == null)
    {
         //your other logic
    }
}
finally {
    sem.Release();
}

// other code

GetOrAdd is an atomic operation, and will always return an object you may lock on.

Of course this approach will work only as long as you have just one instance of your application.

If for any reason you replicate your application into multiple instances (e.g. virtualization, cloud, etc.) then your problem will rise again. In that case I suggest you to take into consideration the use of a distributed lock mechanism (Redis is a good candidate for such a purpose), keeping your logic as is.

Distributed example

Here it is a small example for a distributed approach using StackExchange.Redis, assuming cache is an instance of StackExchange.Redis.IDatabaseAsync:

var timeout = TimeSpan.FromSeconds(5);
var lockKey = clientId + clientSyncTable;
RedisValue lockId = Guid.NewGuid().ToString();

if(await cache.LockTakeAsync(lockKey, lockId, timeout)) { //note that obtaining a distributed lock may fail
    try {
        ISyncMetadataClient syncMetadataClient =
                await SyncDataService.ClientSyncMetadata.GetFirstAsync(
                    new {
                        ClientId = clientId,
                        SyncTable = clientSyncTable
                    }).ConfigureAwait(false);

        if (syncMetadataClient == null)
        {
             //your other logic
        }
    } finally {
        await cache.LockReleaseAsync(key, lockId); //release the same lock
    }
}