C# MultiThread Safe Class Design

Question

I'm trying to designing a class and I'm having issues with accessing some of the nested fields and I have some concerns with how multithread safe the whole design is. I would like to know if anyone has a better idea of how this should be designed or if any changes that should be made?

using System;
using System.Collections;

namespace SystemClass
{
public class Program
{
    static void Main(string[] args)
    {
        System system = new System();

        //Seems like an awkward way to access all the members
        dynamic deviceInstance = (((DeviceType)((DeviceGroup)system.deviceGroups[0]).deviceTypes[0]).deviceInstances[0]);
        Boolean checkLocked = deviceInstance.locked;

        //Seems like this method for accessing fields might have problems with multithreading
        foreach (DeviceGroup dg in system.deviceGroups)
        {
            foreach (DeviceType dt in dg.deviceTypes)
            {
                foreach (dynamic di in dt.deviceInstances)
                {
                    checkLocked = di.locked;
                }
            }
        }
    }
}

public class System
{
    public ArrayList deviceGroups = new ArrayList();

    public System()
    {   
        //API called to get names of all the DeviceGroups
        deviceGroups.Add(new DeviceGroup("Motherboard"));
    }
}

public class DeviceGroup
{
    public ArrayList deviceTypes = new ArrayList();

    public DeviceGroup() {}

    public DeviceGroup(string deviceGroupName)
    {
        //API called to get names of all the Devicetypes
        deviceTypes.Add(new DeviceType("Keyboard"));
        deviceTypes.Add(new DeviceType("Mouse"));
    }
}

public class DeviceType
{
    public ArrayList deviceInstances = new ArrayList();
    public bool deviceConnected;

    public DeviceType() {}

    public DeviceType(string DeviceType)
    {
        //API called to get hardwareIDs of all the device instances
        deviceInstances.Add(new Mouse("0001"));
        deviceInstances.Add(new Keyboard("0003"));
        deviceInstances.Add(new Keyboard("0004"));

        //Start thread CheckConnection that updates deviceConnected periodically
    }

    public void CheckConnection()
    {
        //API call to check connection and returns true
        this.deviceConnected = true;
    }
}

public class Keyboard
{
    public string hardwareAddress;
    public bool keypress;
    public bool deviceConnected;

    public Keyboard() {}

    public Keyboard(string hardwareAddress)
    {
        this.hardwareAddress = hardwareAddress;
        //Start thread to update deviceConnected periodically
    }

    public void CheckKeyPress()
    {
        //if API returns true
        this.keypress = true;
    }
}

public class Mouse
{
    public string hardwareAddress;
    public bool click;

    public Mouse() {}

    public Mouse(string hardwareAddress)
    {
        this.hardwareAddress = hardwareAddress;
    }

    public void CheckClick()
    {
        //if API returns true
        this.click = true;
    }
}

}

Answer 1

Making a class thread-safe is a heck of a difficult thing to do.

The first, naive, way, that many tends to attempt is just adding a lock and ensuring that no code that touches mutable data does so without using the lock. By that I mean that everything in the class that is subject to change, has to first lock the locking object before touching the data, be it just reading from it, or writing to it.

However, if this is your solution, then you should probably not do anything at all to the code, just document that the class is not thread-safe and leave it to the programmer that uses it.

Why?

Because you've effectively just serialized all access to it. Two threads that tries use the class at the same time, even though they are touching separate parts of it, will block. One of the threads will be given access, the other one will wait until the first one is complete.

This is actually discouraging multi-threaded usage of your class, so in this case you're adding overhead of locking to your class, and not actually getting any benefits from it. Yes, your class is now "thread safe", but it isn't actually a good thread-citizen.

The other way is to start adding granular locks, or writing lock-free constructs (seriously hard), so that if two parts of the object aren't always related, code that accesses each part have their own lock. This would allow multiple threads that accesses different parts of the data to run in parallel without blocking one another.

This becomes hard wherever you need to work on more than one part of the data at a time, as you need to be super-careful to take the locks in the right order, or suffer deadlocks. It should be your class' responsibility to ensure the locks are taken in the right order, not the code that uses the class.

As for your specific example, it looks to me as though the parts that will change from background threads are only the "is the device connected" boolean values. In this case I would make that field volatile, and use a lock around each. If, however, the list of devices will change from background threads, you're going to run into problems pretty fast.

You should first try to identify all the parts that will be changed by background threads, and then devise scenarios for how you want the changes to propagate to other threads, how to react to the changes, etc.