current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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;
            }
        }
    }
}
share|improve this question
    
possible duplicate of Creating Thread Safe or Concurrent List –  RubberDuck Sep 5 at 13:22
    
In a sense it is duplicate, but it is using Lock object and I want to understand which would be a better choice –  Mrinal Kamboj Sep 5 at 13:42
1  
What is this thing you call "thread safe"? –  Servy Sep 5 at 17:25
    
If you don't need your list to be ordered, you might want to use the ConcurrentBag<T> –  TopinFrassi Sep 8 at 12:43
    
@TopinFrassi I am aware of ConcurrentBag but in this case I need index based access, that's why ConcurrentBag is ruled out –  Mrinal Kamboj Sep 9 at 5:10

1 Answer 1

up vote 5 down vote accepted

Your new approach is much better than your 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. Your 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 is 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.

share|improve this answer
    
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. –  Mrinal Kamboj Sep 7 at 9:31
    
@MrinalKamboj: I've updated my answer –  ChrisWue Sep 10 at 21:55
    
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. –  svick Sep 11 at 20:22
    
@svick fair enough, fixed –  ChrisWue Sep 12 at 8:31
    
You don't need a lock in the Count property implementation. –  George Polevoy Sep 12 at 16:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.