Locking to make a class threadsafe with a C# example or is this class threadsafe?

Question

I am trying to investigate locking to create a threadsafe class and have a couple of questions. Given the following class:

    public class StringMe 
    {
        protected ArrayList  _stringArrayList = new ArrayList();
        static readonly object _locker = new object();

        public void AddString(string stringToAdd)
        {
            lock (_locker) _stringArrayList.Add(stringToAdd);
        }

        public override string ToString()
        {
            lock (_locker)
            {
return string.Join(",",string[])_stringArrayList.ToArray(Type.GetType("System.String")));
            }
        }
    }

1) Did I successfully make AddString andToString threadsafe?

2) In the ToString method I've created is it necessary to lock there to make it threadsafe?

3) Is it only the methods that modify data that need to be locked or do both the read and write opperations need to be locked to make it threadsafe?

Thank you so much for your time!

Answer 1

No, you haven't made those calls thread-safe - because the _stringArrayList field is protected. Subclasses could be doing whatever they like with it while AddString and ToString are being called.

For example (as the other answers claim that your code is thread-safe.)

public class BadStringMe : StringMe
{
    public void FurtleWithList()
    {
        while (true)
        {
            _stringArrayList.Add("Eek!");
            _stringArrayList.Clear();
        }
    }
}

Then:

BadStringMe bad = new BadStringMe();
new Thread(bad.FurtleWithList).Start();
bad.AddString("This isn't thread-safe");

Prefer private fields - it makes it easier to reason about your code.

Additionally:

• Prefer List<T> to ArrayList these days
• You're locking with a static variable for some reason... so even if you've got several instances of StringMe, only one thread can be in AddString at a time in total
• Using typeof(string) is much cleaner than Type.GetType("System.String")

3) Is it only the methods that modify data that need to be locked or do both the read and write opperations need to be locked to make it threadsafe?

All, assuming that there might be some operations. If everything is just reading, you don't need any locks - but otherwise your reading threads could read two bits of data from the data structure which have been modified in between, even if there's only one writing thread. (There are also memory model considerations to bear in mind.)

Answer 2

1) Did I successfully make AddString andToString threadsafe?

Yes, If you change _stringArrayList to be private

2) In the ToString method I've created is it necessary to lock there to make it threadsafe?

Yes

3) Is it only the methods that modify data that need to be locked or do both the read and write opperations need to be locked to make it threadsafe?

Read and write.