Associating a lock/mutex with the data it protects

Question

I've recently come across some areas of code in our solution that have a few thread-safety problems. Thread-safety is a minefield at best so I thought I'd have an attempt at writing a few little utility classes that would make it easier to write safe multi-threaded code.
The general idea is to link a resource with the lock(s) that protect it. If the resource is accessed outside of all of it's locks then a runtime exception is thrown.

Imagine a developer had written the following (example) caching object but had forgotten to take the _imageLock when clearing the cache.

Using the traditional .net locking mechanism (of just locking on an Object) we might corrupt the Dictionary if two threads call Add() and ClearCache() at the same time.

Using these utility classes, Padlock and Protected<T>, will instead throw an exception in the ClearCache() method because the Protected<T> object is being accessed outside of it's lock. This exception should surface during the developers normal testing procedure, before the code is even checked in.

public class SomeThreadSafeCache
{
    Padlock _imageLock = new Padlock();
    Protected<Dictionary<string, byte[]>> _imageCache;

    public SomeThreadSafeCache()
    {
        //The variable _imageCache is associate with the _imageLock Padlock
        //i.e. _imageCache is only available when we're locking on _imageLock.
        _imageCache = new Protected<Dictionary<string, byte[]>>(
            new Dictionary<string, byte[]>(),
            _imageLock);
    }

    public void AddImage(string key, byte[] data)
    {
        using (_imageLock.Lock())
        {
            _imageCache.Value.Add(key, data);
        }
    }

    public void ClearCache()
    {
        //Throws an exception because the protected
        //resource is accessed outside of it's lock.
        _imageCache.Value.Clear();
    }
}

(I know that .net now comes with Concurrent collections which make my example pointless but the general idea of linking a resource with it's lock is what I'm trying to get at)

The Protected<T> class is able to be associated with as many Padlocks as you might want.

public class SomeOtherThreadSafeObject
{
    Padlock _lock1 = new Padlock();
    Padlock _lock2 = new Padlock();

    Protected<int> _i;

    public SomeOtherThreadSafeObject()
    {
        _i = new Protected<int>(0, _lock1, _lock2);
    }

    public void IncrementI()
    {
        using (_lock1.Lock())
        {
            using (_lock2.Lock())
            {
                _i.Value++;
            }
        }
    }
}

(A trivial example for illustrative purposes only, the Interlocked.Increment function would do a better job.)

If either _lock1 or _lock2 is not taken then

_i.Value++   

will throw an exception stating that the object is not fully locked.

Enforcing the order in which the locks are taken are outside the scope of the utility classes. I'm also trying to avoid the old ICollection.SyncRoot pattern by not allowing

Protected<T>.Padlock.Lock() 

The locking policy itself should be up to the implementors of the class. All Protected<T> is interested is in is knowing if it's fully protected before allowing access to it's internal value.

What I'm asking for a review of is the general approach/idea. Does this seem like a sensible system? Are there any glaring problems with it that I've not thought of? If anyone wants to see the actual implementation code then I can post it on Github (or somewhere similar).

(I'm aware that this abuses the IDisposable/Using pattern but for added thread-safety I think it will be worth it for us).

Answer 1

I confess that I think your approach looks painful. As you can see from the other answers, it's insufficient to protect your data outside the container class. And there's confusion in your example code as to whether Protected is the container or ThreadSafeCache is the container. I think instead that you need to revisit the principle of encapsulation. When you have a collection that requires multi-threaded updates and access, you protect that collection with a wrapper class. The wrapper encapsulates the underlying collection in its entirety. Think "private". If you don't want to expose the lock, then you don't expose an iterator/enumerator. Rather, you do something similar to the List's ForEach method:

public void ForEach(Action<T> t){
   lock(_items) foreach(var item in _items) t.Invoke(item);
}

Answer 2

This looks promising, though without the ability to enforce ordering i would remove the ability to associate multiple locks with a protected object, because you are just asking for a deadlock.

Also, how do you protect against something like this:

Protected<Reference> _i = new Protected<Reference>(0, lock);
Reference unprotected_i;
using (lock.Lock()) 
{
    unprotected_i = _i.Value;
}
unprotected_i.boom();// any modifying operation.

I think that what you would really want to do is create a dynamic proxy object under the hood and return that in the Value getter, then you could enforce the protection.

Answer 3

I am not sure I understand the question. What are these classes going to give me that a standard lock wouldn't?

I would write that first class something like this:

public sealed class SomeThreadSafeCache
{
    static readonly object l = new object();
    static volatile SomeThreadSafeCache instance;

    readonly Dictionary<string, byte[]> _imageCache;

    SomeThreadSafeCache()
    {
        _imageCache = new Dictionary<string, byte[]>();
    }

    public static SomeThreadSafeCache Instance
    {
        get
        {
            if (instance == null)
            {
                lock (l)
                {
                    if (instance == null)
                        instance = new SomeThreadSafeCache();
                }
            }

            return instance;
        }
    }

    public void AddImage(string key, byte[] data)
    {
        if (data == null) 
            throw new ArgumentNullException("data");
        if (key == null) 
            throw new ArgumentNullException("key");

        var v = (byte[]) data.Clone();
        lock (l)
        {
            _imageCache.Add(key, );
        }
    }

    public byte[] GetImage(string key) 
    {
        if (key == null) 
            throw new ArgumentNullException("key");

        byte[] v;
        lock (l)
        {
            _imageCache.TryGetValue(key, out v);
        }
        if (v == null) 
        {
            return null;
        }
        return (byte[]) v.Clone();
    }

    public void ClearCache()
    {
        lock(l)
        {
            _imageCache.Clear();
        }
    }
}

(unable to check correctness right now due to not sitting at my work machine, but barring any syntax errors I think that is right) Am I doing something wrong here?

I think a static constructor is fine for the singleton pattern as well, but I am not 100% sure it covers all thread safety issues. I don't see how a single lock object wouldn't though.

Answer 4

Is there any reason for making "Exception" branch for accessing "Value"? Maybe it's better to use something like

using(var lockedDictionary = _imageCache.Lock())
{
    lockedDictionary.Value.Add(key, data);
}