Creating a thread-safe list using a Lock object

Question

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<T> : IList<T>
{
 private List<T> internalList;
 private readonly object lockList = new object();
 private ThreadSafeListWithLock()
 {
 internalList = new List<T>();
 }
 // Other Elements of IList implementation
 public IEnumerator<T> GetEnumerator()
 {
 return Clone().GetEnumerator();
 }
 System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
 {
 return Clone().GetEnumerator();
 }
 public List<T> Clone()
 {
 ThreadLocal<List<T>> threadClonedList = new ThreadLocal<List<T>>();
 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; }
 }
 public int IndexOf(T item)
 {
 int itemIndex;
 lock ((lockList))
 {
 itemIndex = internalList.IndexOf(item);
 }
 return (itemIndex);
 }
 public void Insert(int index, T item)
 {
 lock ((lockList))
 {
 internalList.Insert(index,item);
 }
 }
 public void RemoveAt(int index)
 {
 lock ((lockList))
 {
 internalList.RemoveAt(index);
 }
 }
 public T this[int index] 
 {
 get
 {
 lock ((lockList))
 { 
 return internalList[index];
 }
 }
 set
 {
 lock ((lockList))
 {
 internalList[index] = value;
 }
 }
 }
}

• RubberDuck: possible duplicate of Creating Thread Safe or Concurrent List
• Mrinal Kamboj: In a sense it is duplicate, but it is using Lock object and I want to understand which would be a better choice
• Servy: What is this thing you call "thread safe"?
• IEatBagels: If you don't need your list to be ordered, you might want to use the ConcurrentBag<T>
• Mrinal Kamboj: @TopinFrassi I am aware of ConcurrentBag but in this case I need index based access, that's why ConcurrentBag is ruled out

Answer 1

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:

1. 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();
   }
   

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

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

   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<T> Clone()
   {
    lock (lockList)
    {
    return new List<T>(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.

• Mrinal Kamboj: Thanks for a detailed explanation and also the explanation of the oddities. Would you think ReaderWriterLockSlim would be a better fit than simple lock. Will it be more efficient.
• ChrisWue: @MrinalKamboj: I've updated my answer
• svick: I think that new List<T>(internalList) would be slightly clearer than ToList() here. ToList() could leave someone wondering "since it's already a list, won't ToList() just return the same instance?" It also better explains the intention: you want to create a new list, that's a copy of the old one, you don't want to convert the old list to list.
• ChrisWue: @svick fair enough, fixed
• George Polevoy: You don't need a lock in the Count property implementation.

Answer 2

Though using same concept, I suggest here different coding approch: By adding 3 methods I query, set values, and get items from the internal list:

These are the utility methods:

protected virtual void LockInternalListAndCommand(Action<IList<T>> action)
 {
 lock (lockObject)
 {
 action(_internalList);
 }
 }
 protected virtual T LockInternalListAndGet(Func<IList<T>, T> func)
 {
 lock (lockObject)
 {
 return func(_internalList);
 }
 }
 protected virtual TObject LockInternalListAndQuery<TObject>(Func<IList<T>, TObject> query)
 {
 lock (lockObject)
 {
 return query(_internalList);
 }
 }

Note: You can use LockInternalListAndQuery ro fetch values from the collectio instead of holding both LockInternalListAndGet and LockInternalListAndQuery. I prefer to keep both methods to have one for pure T object fetching from collectio and second for general purpose (but I believe this is approach).

This is the entire code of my suggestted ConcurrentList object:

 public class ConcurrentList<T> : IList<T>
{
 #region Fields
 private IList<T> _internalList;
 private readonly object lockObject = new object();
 #endregion
 #region ctor
 public ConcurrentList()
 {
 _internalList = new List<T>();
 }
 public ConcurrentList(int capacity)
 {
 _internalList = new List<T>(capacity);
 }
 public ConcurrentList(IEnumerable<T> list)
 {
 _internalList = list.ToList();
 }
 #endregion
 public T this[int index]
 {
 get
 {
 return LockInternalListAndGet(l => l[index]);
 }
 set
 {
 LockInternalListAndCommand(l => l[index] = value);
 }
 }
 public int Count
 {
 get
 {
 return LockInternalListAndQuery(l => l.Count());
 }
 }
 public bool IsReadOnly => false;
 public void Add(T item)
 {
 LockInternalListAndCommand(l => l.Add(item));
 }
 public void Clear()
 {
 LockInternalListAndCommand(l => l.Clear());
 }
 public bool Contains(T item)
 {
 return LockInternalListAndQuery(l => l.Contains(item));
 }
 public void CopyTo(T[] array, int arrayIndex)
 {
 LockInternalListAndCommand(l => l.CopyTo(array, arrayIndex));
 }
 public IEnumerator<T> GetEnumerator()
 {
 return LockInternalListAndQuery(l => l.GetEnumerator());
 }
 public int IndexOf(T item)
 {
 return LockInternalListAndQuery(l => l.IndexOf(item));
 }
 public void Insert(int index, T item)
 {
 LockInternalListAndCommand(l => l.Insert(index, item));
 }
 public bool Remove(T item)
 {
 return LockInternalListAndQuery(l => l.Remove(item));
 }
 public void RemoveAt(int index)
 {
 LockInternalListAndCommand(l => l.RemoveAt(index));
 }
 IEnumerator IEnumerable.GetEnumerator()
 {
 return LockInternalListAndQuery(l => l.GetEnumerator());
 }
 #region Utilities
 protected virtual void LockInternalListAndCommand(Action<IList<T>> action)
 {
 lock (lockObject)
 {
 action(_internalList);
 }
 }
 protected virtual T LockInternalListAndGet(Func<IList<T>, T> func)
 {
 lock (lockObject)
 {
 return func(_internalList);
 }
 }
 protected virtual TObject LockInternalListAndQuery<TObject>(Func<IList<T>, TObject> query)
 {
 lock (lockObject)
 {
 return query(_internalList);
 }
 }
 #endregion
}

• ChrisWue: This interface still isn't thread-safe. You return the enumerator to the caller outside of the lock. So the caller will hold a reference to the underlying list enumerator and modify it outside of the lock. If now another thread modifies the list you will still get an exception. You'll need to implement an enumerator which holds the underlying lock while alive. The other option is to have a `LockAndEnumerate(Action<T> action)' method with enumerates the list while holding the lock calling the callback on every item
• Roi Shabtai: Thank you for your comment. I would check it out and fix the code