Properly locking a List<T> in MultiThreaded Scenarios?

Question

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".

Answer 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().

Answer 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.