Cargo-cult programming: locking on System.Object

Question

I think I've developed a cargo-cult programming habit:

Whenever I need to make a class threadsafe, such as a class that has a Dictionary or List (that is entirely encapsulated: never accessed directly and modified only by member methods of my class) I create two objects, like so:

public static class Recorder {

    private static readonly Object _devicesLock = new Object();
    private static readonly Dictionary<String,DeviceRecordings> _devices;

    static Recorder() {

        _devices = new Dictionary<String,DeviceRecordings>();

        WaveInCapabilities[] devices = AudioManager.GetInDevices();
        foreach(WaveInCapabilities device in devices) {

            _devices.Add( device.ProductName, new DeviceRecordings( device.ProductName ) );
        }
    }//cctor

    // For now, only support a single device.

    public static DeviceRecordings GetRecordings(String deviceName) {

        lock( _devicesLock ) {

            if( !_devices.ContainsKey( deviceName ) ) {

                return null;
            }

            return _devices[ deviceName ];
        }
    }//GetRecordings
}//class

In this case, I wrap all operations on _devices within a lock( _devicesLock ) { block. I'm beginning to wonder if this is necessary. Why don't I just lock on the dictionary directly?

Answer 1

In your use case, locking the dictionary will be fine, since it is private. You still need to carefully design your class to prevent deadlocks.

If the dictionary is the only shared resource that needs thread safety and other parts of your code are thread-safe, I'd recommend using the ConcurrentDictionary instead of locking.

Answer 2

It is not needed if you are sure that the main object is used completely within the class. Strictly speaking it is not necessary even if it is but version with 2 variable is much easier to reason about:

• whoever reads code does not need to think about if the main object is ever exposed and potentially locked by something outised your class
• the code with separate object for locking looks more in line with good practice
• will be safer if one accidentally/intentionally exposes the main object