Properly locking a List<T> in MultiThreaded Scenarios?

Okay, I just can't get my head around multi-threading scenarios properly. Sorry for asking a similar question again, I'm just seeing many different "facts" around the internet.

public static class MyClass {
    private static List<string> _myList = new List<string>;
    private static bool _record;

    public static void StartRecording()
    {
        _myList.Clear();
        _record = true;
    }

    public static IEnumerable<string> StopRecording()
    {
        _record = false;
        // Return a Read-Only copy of the list data
        var result = new List<string>(_myList).AsReadOnly();
        _myList.Clear();
        return result;
    }

    public static void DoSomething()
    {
        if(_record) _myList.Add("Test");
        // More, but unrelated actions
    }
}

The idea is that if Recording is activated, calls to DoSomething() get recorded in an internal List, and returned when StopRecording() is called.

My specification is this:

• StartRecording is not considered Thread-Safe. The user should call this while no other Thread is calling DoSomething(). But if it somehow could be, that would be great.
• StopRecording is also not officially thread-safe. Again, it would be great if it could be, but that is not a requirement.
• DoSomething has to be thread-safe

The usual way seems to be:

    public static void DoSomething()
    {
        object _lock = new object();
        lock(_lock){
            if(_record) _myList.Add("Test");
        }
        // More, but unrelated actions
    }

Alternatively, declaring a static variable:

    private static object _lock;

    public static void DoSomething()
    {
        lock(_lock){
            if(_record) _myList.Add("Test");
        }
        // More, but unrelated actions
    }

However, this answer says that this does not prevent other code from accessing it.

So I wonder

• How would I properly lock a list?
• Should I create the lock object in my function or as a static class variable?
• Can I wrap the functionality of Start and StopRecording in a lock-block as well?
• StopRecording() does two things: Set a boolean variable to false (to prevent DoSomething() from adding more stuff) and then copying the list to return a copy of the data to the caller). I assume that _record = false; is atomic and will be in effect immediately? So normally I wouldn't have to worry about Multi-Threading here at all, unless some other Thread calls StartRecording() again?

At the end of the day, I am looking for a way to express "Okay, this list is mine now, all other threads have to wait until I am done with it".

Solution 1:

I will lock on the _myList itself here since it is private, but using a separate variable is more common. To improve on a few points:

public static class MyClass 
{
    private static List<string> _myList = new List<string>;
    private static bool _record; 

    public static void StartRecording()
    {
        lock(_myList)   // lock on the list
        {
           _myList.Clear();
           _record = true;
        }
    }

    public static IEnumerable<string> StopRecording()
    {
        lock(_myList)
        {
          _record = false;
          // Return a Read-Only copy of the list data
          var result = new List<string>(_myList).AsReadOnly();
          _myList.Clear();
          return result;
        }
    }

    public static void DoSomething()
    {
        lock(_myList)
        {
          if(_record) _myList.Add("Test");
        }
        // More, but unrelated actions
    }
}

Note that this code uses lock(_myList) to synchronize access to both _myList and _record. And you need to sync all actions on those two.

And to agree with the other answers here, lock(_myList) does nothing to _myList, it just uses _myList as a token (presumably as key in a HashSet). All methods must play fair by asking permission using the same token. A method on another thread can still use _myList without locking first, but with unpredictable results.

We can use any token so we often create one specially:

private static object _listLock = new object();

And then use lock(_listLock) instead of lock(_myList) everywhere.

This technique would have been advisable if myList had been public, and it would have been absolutely necessary if you had re-created myList instead of calling Clear().

Solution 2:

Creating a new lock in DoSomething() would certainly be wrong - it would be pointless, as each call to DoSomething() would use a different lock. You should use the second form, but with an initializer:

private static object _lock = new object();

It's true that locking doesn't stop anything else from accessing your list, but unless you're exposing the list directly, that doesn't matter: nothing else will be accessing the list anyway.

Yes, you can wrap Start/StopRecording in locks in the same way.

Yes, setting a Boolean variable is atomic, but that doesn't make it thread-safe. If you only access the variable within the same lock, you're fine in terms of both atomicity and volatility though. Otherwise you might see "stale" values - e.g. you set the value to true in one thread, and another thread could use a cached value when reading it.

Solution 3:

There are a few ways to lock the list. You can lock on _myList directly providing _myList is never changed to reference a new list.

lock (_myList)
{
    // do something with the list...
}

You can create a locking object specifically for this purpose.

private static object _syncLock = new object();
lock (_syncLock)
{
    // do something with the list...
}

If the static collection implements the System.Collections.ICollection interface (List(T) does), you can also synchronize using the SyncRoot property.

lock (((ICollection)_myList).SyncRoot)
{
    // do something with the list...
}

The main point to understand is that you want one and only one object to use as your locking sentinal, which is why creating the locking sentinal inside the DoSomething() function won't work. As Jon said, each thread that calls DoSomething() will get its own object, so the lock on that object will succeed every time and grant immediate access to the list. By making the locking object static (via the list itself, a dedicated locking object, or the ICollection.SyncRoot property), it becomes shared across all threads and can effectively serialize access to your list.

Solution 4:

The first way is wrong, as each caller will lock on a different object. You could just lock on the list.

lock(_myList)
{
   _myList.Add(...)
}