pvginkel / inmemcached Goto Github PK

I'm trying to review the code to try to ensure that there are no gremlins hiding in the multi-threaded complexity.. and I'm finding it quite difficult! Let me know if any of what I've written here seems incorrect..

In the "Add" method, you're not locking the handle when the _cache.TryAdd call fails since no other threads are able to get a reference to that handle and so there will never be multiple threads trying to manipulate it. This is unlike in "Get" and "Remove" where locking is required because it's possible for one thread to enter the "_cache.TryGetValue" in "Get" condition and for another to enter the "_cache.TryRemove" condition in "Remove" simultaneously; in this case, only one of these operations may be allowed at a time (and if "Remove" gets there first then "Get" will have to return null since the handle has been free'd, which is why "ToByteArray" may return null).

I presume that you're applying similar logic to the lock in the "Dispose" method. However, if that is the case then there is a potential race condition. The "Values" property on the ConcurrentDictionary is a snapshot of Values that are present at the point at which the property is accessed. If any values are removed from the dictionary during the enumeration, this will not affect the values that are enumerated over. For example, if there are three handles to enumerate over and the third handle is removed by a "Remove" call on a thread that executes while another thread is already enumerating the dictionary values from within "Dispose" then "Dispose" will try to Free the handle that "Remove" has already free'd.

This is illustrated with the below example:

class Program
{
    static void Main(string[] args)
    {
        var cache = new ConcurrentDictionary<int, MemoryHandle>();
        cache.TryAdd(1, new MemoryHandle());
        new Thread(() =>
        {
            foreach (var value in cache.Values)
            {
                // Introduce a short delay to ensure that the TryRemove call below completes
                // before this enumeration completes
                Thread.Sleep(TimeSpan.FromSeconds(1));
                value.Free();
            }
        }).Start();
        new Thread(() =>
        {
            MemoryHandle removedHandle;
            if (cache.TryRemove(1, out removedHandle))
                removedHandle.Free();
        }).Start();
        Console.ReadLine();
    }

    private sealed class MemoryHandle
    {
        private bool _freed = false;
        public void Free()
        {
            if (_freed)
                throw new Exception("This handle has already been free'd!");
            _freed = true;
        }
    }
}

Maybe it would be best to change "Free" to work like "ToByteArray" - in that it will perform no work if the handle is a null pointer?