4

We have a MVC Web API that is called to write transactions for a specific amount, where the amount is debited or credited to a user's balance. This records the balance pre-transaction (PreAmount), the amount, and the balance post-transaction (PostAmount). We noticed from a simple query using SQL Server's LAG and LEAD functions that there were instances of transactions being added from the API were being 'entered too fast' (partially due to the lack of resources put towards buying good hardware for the server, and IIS and SQL running on the same box). We have a mechanism to prevent the transaction from being entered if it exists, but it seems that the method call is being called really quickly, which results in the record being added multiple times due to a race condition.

The way I am going to try to fix this is by using a Dictionary<long, object>. The assumption here is that, we want transactions for a customer to be entered sequentially at all times, but I don't want to lock concurrent requests for multiple customers. The dictionary would contain a customer ID # and the value would be an object I can lock on. When a transaction is about to be entered, I will check to see if there is a customer ID # entry, and if not, enter it in there):

private static Dictionary<long, object> customerLocker = new Dictionary<long, object>();
private static object dictLocker = new object();

Then, when a request to check a balance, or to enter a transaction is requested, I would then lock on that object, and perform the work needed:

lock (dictLocker)
{
    if (!customerLocker.ContainsKey(customerID))
        customerLocker.Add(customerID, new object());
}
lock (customerLocker[customerID])
{
    // ... do the work needed for this customer
}

Is this a good solution to the issue I am having? Will locking on elements inside of a dictionary cause issues? I'm going to try it, but while I'm putting this together, I'd like to see input from others on this scenario.

migrated from codereview.stackexchange.com Dec 2 '16 at 19:54

This question came from our site for peer programmer code reviews.

5

It's not safe for you to be searching through the dictionary outside of a lock, because by doing so another dictionary could be writing to it at the same time, so your read could end up being corrupted. You need to fetch the value for the key inside of the lock.

Alternatively, you could just use a ConcurrentDictionary to store the objects for each customer instead of locking around access to a Dictionary. This would let you write:

private static ConcurrentDictionary<long, object> customerLocker = 
    new ConcurrentDictionary<long, object>();
lock (customerLocker.GetOrAdd(customerID, () => new object()))
{
    // ... do the work needed for this customer
}
  • That's why I have dictLocker. Is that not enough for a standard dictionary to use? – Derreck Dean Dec 2 '16 at 19:27
  • Ohhhhhh. I see what you mean now. – Derreck Dean Dec 2 '16 at 19:28
  • 1
    @DerreckDean If you properly lock around all access to it (your current code does not) then it would work. ConcurrentDictionary would provide multiple advantages, 1) it means you don't need to deal with the explicit locking, you can let it handle the synchronization for you 2) it's likely to do so more efficiently, because, much like your code does for bank accounts, it can avoid synchronizing everything anytime anything accesses the dictionary, and instead synchronize around only what it needs to, and allow operations to happen concurrently so long as it can prove that it's done safely. – Servy Dec 2 '16 at 19:29
  • Added this code earlier and it works as expected. Race conditions are gone. – Derreck Dean Dec 2 '16 at 22:00

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.