0

My challenge:

  1. avoid race conditions in a method DoWorkWithId that's being triggered from the UI by multiple users for multiple ids, e.g. lock
  2. re-entrance into DoWorkWithId permitted for different ids but not for the same id , e.g. using a ConcurrentDictionary where id is the key and value is an object
  3. should be a non-blocking lock so threads skip critical section without waiting and letting others know that it's already running (at least was running when invoked a second time), e.g. Monitor.TryEnter or Interlocked.*

My attempt:

I guess 1. and 2. could be solved using a ConcurrentDictionary and lock

private static readonly ConcurrentDictionary<Guid, object> concurrentDictionaryMethodLock = new();
public string CallToDoWorkWithId(Guid id) // [Edited from DoWorkWithId]
{
    concurrentDictionaryMethodLock.TryAdd(id, new object()); // atomic adding
    lock (concurrentDictionaryMethodLock[id])
    {
        DoWorkWithId(id);
    }
    return "done";
}

Now, I don't want threads with the same id to wait. When the first thread is done, threads that waited would find out in the first line of DoWorkWithId that there's nothing to do as the object id refers to has been already modified.

I was hoping to tackle 3. with Monitor.TryEnter or Interlocked.*

3.1. MSDN says lock's basically

object __lockObj = x;
bool __lockWasTaken = false;
try
{
    System.Threading.Monitor.Enter(__lockObj, ref __lockWasTaken);
    // Your code...
}
finally
{
    if (__lockWasTaken) System.Threading.Monitor.Exit(__lockObj);
}

3.2. MSDN example of Monitor.TryEnter

var lockObj = new Object();
bool lockTaken = false;

try
{
    Monitor.TryEnter(lockObj, ref lockTaken);
    if (lockTaken)
    {
        // The critical section.
    }
    else
    {
        // The lock was not acquired.
    }
}
finally
{
    // Ensure that the lock is released.
    if (lockTaken)
    {
        Monitor.Exit(lockObj);
    }
}

In order to correctly release the lock I thought I need to keep track who's acquired the lock, e.g. private static readonly ConcurrentDictionary<Guid, bool> bools = new();.

So, I tried:

    private static readonly ConcurrentDictionary<Guid, object> concurrentDictionaryMethodLock = new();
    private static readonly ConcurrentDictionary<Guid, bool> bools = new();
    public string CallToDoWorkWithId(Guid id) // [Edited from DoWorkWithId]
    {
        concurrentDictionaryMethodLock.TryAdd(id, new object());
        bools.TryAdd(id, false);
        try
        {
            Monitor.TryEnter(concurrentDictionaryMethodLock[id], ref bools[id]); // error
            if (bools[id])
            {
                DoWorkWithId(id);
            }
            else
            {
                return "Process already running. Wait, refresh page and try again.";
            }
        }
        finally
        {
            if (bools[id])
            {
                Monitor.Exit(concurrentDictionaryMethodLock[id]);
            }
        }
        return "done";
    }

This gives me CE CS0206: A property or indexer may not be passed as an out or ref parameter.

Same thing when using Interlocked.*. I need to keep track of the id based booleans, here called usingResource, MSDN example.

My question:

How can I use the verbose syntax of lock and store the bools used to identify who has the lock/ressource when using a lock on a ConcurrentDictionary?

PS:

I'm also interested in better suited solutions for what I'm trying to do. I was thinking of using a ConcurrentDictionary without a lock:

private static readonly ConcurrentDictionary<Guid, string> concurrentDictionaryMethodLock = new();
public string CallToDoWorkWithId(Guid id) // [Edited from DoWorkWithId]
{
    var userName = GetUserName();
    if (!concurrentDictionaryMethodLock.TryAdd(id, userName)) // false means unable to add, was already added, please skip and let UI know
    {
        if (concurrentDictionaryMethodLock.TryGetValue(id, out var value))
        {
            return $"Please wait. {value} started process already.";
        }
        else // in case id has been removed in the meantime, even TryGetOrAdd is not atomic (MSDN remarks about valueFactory) 
        {
            return "Process was running and finished by now. Please refresh and try again.";
        }
    }
    try
    {
        DoWorkWithId(id);
    }
    finally
    {
        concurrentDictionaryMethodLock.TryRemove(id, out _);
    }
    return "done";
}

Any pitfalls using this approach?

NTobiasK
  • 21
  • 3
  • do you need the inner TryGetValue if-statement? It seems you're mixing responsibilities here.The rest of the method should successfully process each item only once at the same time. – Lasse V. Karlsen Apr 27 '21 at 15:47
  • Thanks, the TryGetValue is not required but would be nice so I can show a more meaningful message to other users trying to run the process. That's why I want to access the value (user name) and TryAdd doesn't return the value so I thought I need it to access the value and the pair could be removed meanwhile. – NTobiasK Apr 27 '21 at 15:52
  • This seems overly complicated. Why not just store a boolean in your main `ConcurrentDictionary` object that says whether it's in use. So for example if your key/value pair is a guid/object like in your example, add a property to the object which indicates if it's in use. When someone accesses that guid, set it to true, when it's done being processed set it to false, when another user tries accessing that object it will show that it is currently processing and you can use a simple `if` condition to deny access. Locking on a `ConcurrentDictionary` seems counter intuitive. – Ryan Wilson Apr 27 '21 at 15:56
  • Thanks Ryan, that totally makes sense. Changing the object's bool using TryUpdate is similar to CompareExchange ([MSDN][1]) so it's atomic and safe to use. locking the ConcurrentDictionary arose as I was trying to do a conditional locking. [1] https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=net-5.0 – NTobiasK Apr 27 '21 at 16:23
  • You could take a look at this question: [How to dynamically lock strings but remove the lock objects from memory](https://stackoverflow.com/questions/33786579/how-to-dynamically-lock-strings-but-remove-the-lock-objects-from-memory). It includes an answer with a `KeyedMonitor` implementation, that has methods `Enter`, `TryEnter` and `Exit`. – Theodor Zoulias Apr 27 '21 at 16:49
  • To clarify my response to Ryan: Actually, changing the bool property of the object is not atomic. TryUpdate is atomic on the object but there's no thread safe mechanism to change the object's bool. Though, I could use an immutable object and replace it during TryUpdate (https://stackoverflow.com/questions/47030692/c-sharp-concurrentdictonary-how-to-update-value-properties). – NTobiasK Apr 27 '21 at 18:57

0 Answers0