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;
            }
        }
    }
}

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.

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
}