NamedLock looks unsafe for concurrent use

[vgrudenic]

Lock removal at line 75, currently looks like this:

if (padlock.Decrement () <= 0)
    m_waitLock.TryRemove (key, out padlock);

Since this operation is not atomic, it is possible for a different thread to acquire the padlock before the if statement, but increment it after the if statement. So, thread T1 will remove the padlock while thread T2 starts the critical section, meaning that if a third thread enters GetOrAdd while T2 is working, it will get a new padlock instance.

To avoid a global lock, perhaps having a while loop inside GetOrAdd would ensure that the incremented lock wasn't removed from the dictionary:

private static object GetOrAdd (string key)
{
    CountedLock padlock = null;

    while (true)
    {
        padlock = m_waitLock.GetOrAdd (key, LockFactory);
        padlock.Increment ();

        // double check that things haven't changed
        if (padlock == m_waitLock.GetOrAdd (key, LockFactory))
            return padlock;

        // oops
        padlock.Decrement ();
    };
}

This is off top of my head, I still have to check a bit more if it makes sense.

[khalidsalomao]

Hi @vgrudenic, thank you for pointing this out. This section is vulnerable to a race condition that could leave an active lock out of the internal collection.

I will take a look into this!