Dynamic Multi-threading lock with while loop

Question

I have already asked a question about a dynamic multi threaded lock, and the reviewer found a problem.

Here is the description of the problem that I need to solve:

I need to create a structure to control dynamically created locks. For example, I have 100 threads running, and each thread will "process" a "key". Sometimes two threads could be processing the same "key", and if that occurs, a dynamic lock needs to be created and used by both, so only one thread can process the key at a time.

I rewrote my code again with a new idea, and I would like you to review it for possible bugs/leaks caused by multithreaded processing.

//Implementation of the class who will block to two process don't execute if they have the same key.
public class DynamicLockGenerator
{
    private static Dictionary<string, object> _locksByKeys = new Dictionary<string, object>();

    private static object _lock = new object();

    public static void ExecuteLock(string key, Action action)
    {
        try
        {
            bool isFree = true;

            do
            {
                lock (_lock)
                {
                    if (!_locksByKeys.ContainsKey(key))
                        _locksByKeys.Add(key, new GuidLock(Guid.NewGuid()));

                    isFree = Monitor.TryEnter(_locksByKeys[key]);
                }

                if (!isFree)
                {
                    Thread.Sleep(50);
                }

            } while (!isFree);

            action.Invoke();
        }
        catch (Exception ex)
        {
            throw ex;
        }
        finally
        {
            Monitor.Exit(_locksByKeys[key]);
        }
    }
}



//This is the method call.
DynamicLockGenerator.ExecuteLock(key, () => SomeMethodToExecuteAlone());

Answer 1

Just one question : when do you release the key ? Suppose twice the same key is called almost simultaneously, so that one of the two threads involved will go through the

Thread.Sleep(50) ;

line .. The first thread enters the lock, the key is added to the dictionary, fine. At almost the same time the other thread enters the lock, find the key already there and go sleeping. When the first thread exit, if you don't release the key, the other thread will wait a long long time .. May be something like

_lockByKeys.Remove(key)

is enough to solve this one.

After this, reading again this code, I found a dictionary, a lock and a monitor. I'm quite sure that a ConcurrentDictionary could make the same job, with a cleaner code if not faster.

Lastly, 100 threads with some of them blocked could become a production nightmare if too much keys have the same value.

Hope this help !

Answer 2

As has been said, you're not removing items from your _locksByKeys. As a result of this, your locking mechanism as it stands looks like it will work.

Don't do this:

catch (Exception ex)
{
    throw ex;
}

If you're going to ignore the exception, just don't catch it. It has the same behaviour (you can have a finally without a catch block).

Your try/catch/finally block starts before you've claimed the resource from your dictionary. Since the finally block releases a lock, it should only be in scope after the lock has been obtained:

try
{
    action.Invoke();
}
finally
{
    Monitor.Exit(_locksByKeys[key]);
}

Depending on what your application does you may be better off using Thread.Yield, rather than Thread.Sleep.

I've mocked up an alternate implementation using ConcurrentDictionary ( I haven't extensively tested it). Essentially it works on the premiss that if a thread manages to add the key to the dictionary, it has locked that entry, so can do some processing and then remove the entry.

private static ConcurrentDictionary<string, Guid> _locksByKeys = new
                                                     ConcurrentDictionary<string, Guid>();

public static void ExecuteLock(string key, Action action)
{
    // Declare unique key, mostly for testing purposes
    var guid = Guid.NewGuid();

    // Loop, trying to add key to dictionary.  If the add fails then yield
    // or sleep the thread.  If it adds, then we effectively own the lock.
    while (!_locksByKeys.TryAdd(key, guid)) Thread.Yield();

    // Run the action
    try
    {
        action.Invoke();
    }
    finally
    {
        // Remove the key from the dictionary.  For testing, this validates
        // that the GUID associated with the key hasn't changed and that
        // the remove succeeded
        Guid guidOut;
        if (!_locksByKeys.TryRemove(key, out guidOut) || guidOut != guid)
        {
            throw new InvalidOperationException("It all went wrong");
        }
    }
}