Creating a thread-safe list using a Lock object

Will this code qualify for a truly thread-safe list? It is using the Lock object.

```
using System;
using System.Collections.Generic;
using System.Threading;

public class ThreadSafeListWithLock : IList
{
private List internalList;

private readonly object lockList = new object();

private ThreadSafeListWithLock()
{
internalList = new List();
}

// Other Elements of IList implementation

public IEnumerator GetEnumerator()
{
return Clone().GetEnumerator();
}

System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
{
return Clone().GetEnumerator();
}

public List Clone()
{
ThreadLocal> threadClonedList = new ThreadLocal>();

lock (lockList)
{
internalList.ForEach(element => { threadClonedList.Value.Add(element); });
}

return (threadClonedList.Value);
}

public void Add(T item)
{
lock (lockList)
{
internalList.Add(item);
}
}

public bool Remove(T item)
{
bool isRemoved;

lock (lockList)
{
isRemoved = internalList.Remove(item);
}

return (isRemoved);
}

public void Clear()
{
lock (lockList)
{
internalList.Clear();
}
}

public bool Contains(T item)
{
bool containsItem;

lock (lockList)
{
containsItem = internalList.Contains(item);
}

return (containsItem);
}

public void CopyTo(T[] array, int arrayIndex)
{
lock (lockList)
{
internalList.CopyTo(array,arrayIndex);
}
}

public int Count
{
get
{
int count;

lock ((lockList))
{
count = internalList.Count;
}

return (count);
}
}

public bool IsReadOnly
{
get { return false; }

Your new approach is much better than the previous one. Instead of deriving from List and using new to hide the base class implementations you create a wrapper around a List implementing IList - this is a much more robust way.

However there are a few oddities:

-
You don't need to capture the result of a function call in a local variable to return it outside of the lock. You can simply do:

lock (myLock)
{
    return SomeFunction();
}

-
The Clone implementation is using a ThreadLocal object which is not necessary. For example:

void DoSomething()
{
     var list = new List();
}

If two threads call DoSomething at the same time they will not share list - each will get their own list. So Clone can be shortened to:

public List Clone()
{
    lock (lockList)
    {
        return new List(internalList);
    }
}

(the LINQ ToList() extension method creates a new copy).

Update: As for your question whether using a ReaderWriterLockSlim would be better - it depends. Using a reader-writer lock is more complex because now you have to make sure you correctly obtain the read/write lock in the correct places. Using a plain lock will be sufficient in 99.99% of all cases and should only be optimized if you actually have proof that it causes a performance problem.

The lock statement also has nice compiler support which will generate the try-catch block around it to make sure it's released when anything throws - something you have to do yourself if you use another lock.

And last but not least I'm suspicious of your need for a thread-safe list. The main feature of IList is the random access capabilities for reading/writing/inserting/removing items at a specific index. However in a multi threaded world these features are less useful than you might think. For example doing this:

if (list.Count > indexImInterestedIn)
{
    DoSomething(list[indexImInterestedIn]);
}

is not thread safe because between the conditional check and the access something could have easily removed elements from the list. So the above code can fail even though the individual operations are thread-safe.

If you check your use-case for the thread-safe list you will probably find that you won't actually need an IList.

StackExchange Code Review Q#62033, answer score: 21