5
\$\begingroup\$

I need to create an structure to control dynamically 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 it occurs, a dynamic lock needs to be created and used by both, to process one at a time.

I create the following structure and I would like a code review, to find possible bugs (even if it is extremely rare):

  // This is the class who control to create one different lock for each key.
    public class DynamicLockGenerator
{
    private static Dictionary<string, object> _locksByKeys = new Dictionary<string, object>();

    private static object _lock = new object();

    public static objectGetLock(string key)
    {
        lock (_lock)
        {
            if (!_locksByKeys.ContainsKey(key))
                _locksByKeys.Add(key, new object());

            return _locksByKeys[key];
        }
    }

    public static void RemoveLock(string key)
    {
        lock (_lock)
        {
            //This line is extremely important: One lock object can only be removed
            //if no thread are using it.
            if (Monitor.TryEnter(_locksByKeys[key]))
            {
                _locksByKeys.Remove(key);
            }
        }
    }
}

//This is the use of the class. All threads do the same thing, but with different (or equal) keys.
object lockObj = DynamicLockGenerator.GetLock(key);

lock (lockObj)
{
    //PROCESS THE KEY
}

DynamicLockGenerator.RemoveLock(key);
\$\endgroup\$
1
  • \$\begingroup\$ Maybe you could just use the key as lock object!? Using string.intern ensures that you get always the same instance of the string. \$\endgroup\$
    – JanDotNet
    Commented May 17, 2016 at 14:09

1 Answer 1

3
\$\begingroup\$

I'm pretty sure you have the potential for a race condition if one thread releases the lock and removes it, after another thread has got the lock but not yet locked it.

Thread one GetLock(someKey) // Creates lock, returns dynamic key
Thread one Lock(dynamicKey)
Thread one complete processing (exits lock section)
Thread two GetLock(someKey) // returns existing key
Thread one RemoveLock // removes lock from collection (not yet being used)
Thread two Lock(dynamicKey) // Now we've locked an object no longer in collection
Thread two complete processing (exit lock block)
Thread two Remove lock // tries to remove a lock that's not in the collection
\$\endgroup\$
6
  • \$\begingroup\$ Thank you by the answer, you are right. But could you help-me how could I do this dynamic lock? No answers of stackoverflow and google helps-me until now. \$\endgroup\$ Commented May 17, 2016 at 16:44
  • 1
    \$\begingroup\$ @OnlyaCuriousMind The easiest way to fix the problem I've highlighted is to never remove the lock objects from _locksByKeys. Whether or not that's practical obviously depends on the number of unique keys you're expecting and how long running the process is that your code will sit in. \$\endgroup\$
    – forsvarir
    Commented May 17, 2016 at 16:49
  • \$\begingroup\$ I have alread thought about this, but I have a looooooooot of keys to process, so if I do this memory leak could appear. \$\endgroup\$ Commented May 17, 2016 at 16:50
  • \$\begingroup\$ I had an idea, I am developing now and I will edit my question later. \$\endgroup\$ Commented May 17, 2016 at 16:52
  • 1
    \$\begingroup\$ @OnlyaCuriousMind I'm fairly new to code review, but I think the preferred process is for you to leave your current question as it is and submit any subsequent versions as a new question, that way the code and reviews don't get out of sync. \$\endgroup\$
    – forsvarir
    Commented May 17, 2016 at 16:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.