Multi-Threading Question - adding an Element to a static List

Question

Okay, newbie multi-threading question:

I have a Singleton class. The class has a Static List and essentially works like this:

class MyClass {
    private static MyClass _instance;
    private static List<string> _list;
    private static bool IsRecording;

    public static void StartRecording() {
        _list = new List<string>();
        IsRecording = true;
    }

    public static IEnumerable<string> StopRecording() {
        IsRecording = false;
        return new List<string>(_list).AsReadOnly();
    }

    public MyClass GetInstance(){

    }

    public void DoSomething(){
        if(IsRecording) _list.Add("Something");
    }
}

Basically a user can call StartRecording() to initialize a List and then all calls to an instance-method may add stuff to the list. However, multiple threads may hold an instance to MyClass, so multiple threads may add entries to the list.

However, both list creation and reading are single operations, so the usual Reader-Writer Problem in multi-threading situations does not apply. The only problem I could see is the insertion order being weird, but that is not a problem.

Can I leave the code as-is, or do I need to take any precautions for multi-threading? I should add that in the real application this is not a List of strings but a List of Custom Objects (so the code is _list.Add(new Object(somedata))), but these objects only hold data, no code besides a call to DateTime.Now.

Edit: Clarifications following some answers: DoSomething cannot be static (the class here is abbreviated, there is a lot of stuff going on that is using instance-variables, but these created by the constructor and then only read). Is it good enough to do

lock(_list){
    _list.Add(something);
}

and

lock(_list){
    return new List<string>(_list).AsReadOnly();
 }

or do I need some deeper magic?

Answer 1

You certainly must lock the _list. And since you are creating multiple instances for _list you can not lock on _list itself but you should use something like:

private static object _listLock = new object();

As an aside, to follow a few best practices:

• DoSomething(), as shown, can be static and so it should be.

• for Library classes the recommended pattern is to make static members thread-safe, that would apply to StartRecording(), StopRecording() and DoSomething().

I would also make StopRecording() set _list = null and check it for null in DoSomething().

And before you ask, all this takes so little time that there really are no performance reasons not to do it.

Answer 2

You need to lock the list if multiple threads are adding to it.

A few observations...

Maybe there's a reason not to, but I would suggest making the class static and hence all of its members static. There's no real reason, at least from what you've shown, to require clients of MyClass to call the GetInstance() method just so they can call an instance method, DoSomething() in this case.

I don't see what prevents someone from calling the StartRecording() method multiple times. You might consider putting a check in there so that if it is already recording you don't create a new list, pulling the rug out from everyone's feet.

Finally, when you lock the list, don't do it like this:

static object _sync = new object();
lock(_sync){
    _list.Add(new object(somedata));
}

Minimize the amount of time spent inside the lock by moving the new object creation outside of the lock.

static object _sync = new object();
object data = new object(somedata);
lock(_sync){
    _list.Add(data);
}

EDIT

You said that DoSomething() cannot be static, but I bet it can. You can still use an object of MyClass inside DoSomething() for any instance-related stuff you have to do. But from a programming usability perspective, don't require the users to MyClass to call GetInstance() first. Consider this:

class MyClass {
    private static MyClass _instance;
    private static List<string> _list;
    private static bool IsRecording;
    public static void StartRecording()
    {
        _list = new List<string>();
        IsRecording = true;
    }
    public static IEnumerable<string> StopRecording()
    {
        IsRecording = false;
        return new List<string>(_list).AsReadOnly();
    }
    private static MyClass GetInstance() // make this private, not public
    {   return _instance;    }
    public static void DoSomething()
    {
        // use inst internally to the function to get access to instance variables
        MyClass inst = GetInstance();
    }
}

Doing this, the users of MyClass can go from

MyClass.GetInstance().DoSomething();

to

MyClass.DoSomething();

Answer 3

.NET collections are not fully thread-safe. From MSDN: "Multiple readers can read the collection with confidence; however, any modification to the collection produces undefined results for all threads that access the collection, including the reader threads." You can follow the suggestions on that MSDN page to make your accesses thread-safe.

One problem that you would probably run into with your current code is if StopRecording is called while some other thread is inside DoSomething. Since creating a new list from an existing one requires enumerating over it, you are likely to run into the old "Collection was modified; enumeration operation may not execute" problem.

The bottom line: practice safe threading!