Thread-safe List<T> property

Question

I want an implementation of List<T> as a property which can be used thread-safely without any doubt.

Something like this:

private List<T> _list;

private List<T> MyT
{
    get { // return a copy of _list; }
    set { _list = value; }
}

It seems still I need to return a copy (cloned) of collection so if somewhere we are iterating the collection and at the same time the collection is set, then no exception is raised.

How to implement a thread-safe collection property?

Solution 1

If you are targetting .Net 4 there are a few options in System.Collections.Concurrent Namespace

You could use ConcurrentBag<T> in this case instead of List<T>

Solution 2

Even as it got the most votes, one usually can't take System.Collections.Concurrent.ConcurrentBag<T> as a thread-safe replacement for System.Collections.Generic.List<T> as it is (Radek Stromský already pointed it out) not ordered.

But there is a class called System.Collections.Generic.SynchronizedCollection<T> that is already since .NET 3.0 part of the framework, but it is that well hidden in a location where one does not expect it that it is little known and probably you have never ever stumbled over it (at least I never did).

SynchronizedCollection<T> is compiled into assembly System.ServiceModel.dll (which is part of the client profile but not of the portable class library).

Solution 3

I would think making a sample ThreadSafeList class would be easy:

public class ThreadSafeList<T> : IList<T>
{
    protected List<T> _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();
    }

    protected static object _lock = new object();

    public List<T> Clone()
    {
        List<T> newList = new List<T>();

        lock (_lock)
        {
            _internalList.ForEach(x => newList.Add(x));
        }

        return newList;
    }
}

You simply clone the list before requesting an enumerator, and thus any enumeration is working off a copy that can't be modified while running.

Solution 4

Even accepted answer is ConcurrentBag, I don't think it's real replacement of list in all cases, as Radek's comment to the answer says: "ConcurrentBag is unordered collection, so unlike List it does not guarantee ordering. Also you cannot access items by index".

So if you use .NET 4.0 or higher, a workaround could be to use ConcurrentDictionary with integer TKey as array index and TValue as array value. This is recommended way of replacing list in Pluralsight's C# Concurrent Collections course. ConcurrentDictionary solves both problems mentioned above: index accessing and ordering (we can not rely on ordering as it's hash table under the hood, but current .NET implementation saves order of elements adding).

Solution 5

C#'s ArrayList class has a Synchronized method.

var threadSafeArrayList = ArrayList.Synchronized(new ArrayList());

This returns a thread safe wrapper around any instance of IList. All operations need to be performed through the wrapper to ensure thread safety.

View more solutions

Comments

[atoMerz]

use locks, that should do it.

[Greg]

Can use use a thread-safe implementation of IList<T> (vs List<T>)?

[Roi Shabtai]

Have you checked SynchronizedCollection<T> ?

[kumar chandraketu]

Use BlockingCollection or ConcurrentDictionary

[Victor Yarema]

What operations you need to do with object behind the property? Is it possible that you don't need everything that List<T> implements? If yes, then could you please provide an interface that you need instead of asking about everything that List<T> already have?

[thomasgalliker]

This is probably the question all good programmers sooner or later ask in their life. Using lock(…) around all List operations seems the right answer.

[Tejs]

That's not going to solve his problem; it only stops threads from setting the reference, not adding to the list.

[Xaqron]

And what if one thread is setting the value while another is iterating the collection (it's possible with your code).

[Josh M.]

Like I said, the lock will probably have to be moved out further in the code. This is just an example of how to use the lock statement.

[Joel Mueller]

Yeah, except it's an example of how not to use the lock statement. I can create a complete deadlock and freeze the program with hardly any effort (or by accident) with that example: var myRef = foo.MyT; lock(myRef) { foo.MyT = myRef; }

[Josh M.]

@Joel Mueller: Sure, if you manufacturer some silly example like that. I'm just trying to illustrate that the asker should look into the lock statement. Using a similar example I could argue that we shouldn't use for loops since you could deadlock the application with hardly any effort: for (int x = 0; x >=0; x += 0) { /* Infinite loop, oops! */ }

[Josh M.]

@Joel Mueller: Just to summarize, the code I posted will NOT deadlock the application because the object used in the lock statement is being set on the same thread. If you'd like, I can post an example showing this.

[Joel Mueller]

I never claimed your code meant instant deadlock. It's a bad answer to this particular question for the following reasons: 1) It doesn't protect against the contents of the list being modified during enumeration of the list, or by two threads at once. 2) Locking the setter but not the getter means the property is not really thread-safe. 3) Locking on any reference that is accessible from outside the class is widely considered a bad practice, as it dramatically increases the chances of accidental deadlock. That's why lock (this) and lock (typeof(this)) are big no-no's.

[The Light]

Like List<T> and unlike Dictionary, ConcurrentBag accepts duplicates.

[Joel B]

Isn't this a shallow clone? If T is a reference type, won't this just return a new list containing references to all the original objects? If that is the case, this approach could still cause threading trouble since the list objects could be accessed by multiple threads through different "copies" of the list.

[Tejs]

Correct, it is a shallow copy. The point was to simply have a cloned set that would be safe to iterate over (so newList does not have any items added or removed which would invalidate the enumerator).

[Joel B]

Roger that. This is a really elegant solution to this problem. After some serious thinking/Googl-ing I haven't really turned up anything else that comes close to this in terms of balancing safety/simplicity. Thanks for posting this!

[Radek Stromský]

ConcurrentBag is unordered collection, so unlike List<T> it does not guarantee ordering. Also you cannot access items by index.

[Mike Ward]

Should the _lock be static?

[Mike Ward]

Another thought. Is this implementation threadsafe for multiple writers? If not, maybe it should be called a ReadSafeList.

[Caio Cunha]

@RadekStromský is right, and in the case you wanna an ordered concurrent list, you could try ConcurrentQueue (FIFO) or ConcurrentStack (LIFO).

[Josh M.]

@MikeWard - I don't think it should be, all instance will lock when any instance is being cloned!

[Cœur]

r-h made a similar implementation but where the (private?) method IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); } will not return a Clone(). Is that an improvement or an issue? I don't know which one is best.

[Tejs]

Yes - locking on the internal list directly would be better; otherwise all instances lock when a single instance clones. As for the enumerator call, it's cloned so that when enumerating, if a new element is added to the list while another thread is enumerating, it doesn't blow the enumerator out because the underlying collection was modified during enumeration.

[ANeves]

This is not a clone, it's a copy. You really should rename it. Maybe create a copy constructor, like List<T> has.

[ANeves]

Not true at all. You can use Concurrent sets.

[Fka]

And ConcurrentBag creates copy of its collection for each thread so this might be misleading. Also operation such as removing and adding behaves diffrent.

[deniz]

Additional helpful discussion of this option: stackoverflow.com/a/4655236/12484

[Roi Shabtai]

Maybe SynchronizedCollection<T> ?

[Mr Anderson]

Your Clone() method could simply be lock(_lock) { return new List<T>(_internalList); }

[tytyryty]

please provide reasons for -1

[Xaqron]

I didn't down-vote and there is no reason for it IMO. You are right but the concept is already mentioned in some answers. To me, the point was there is an new thread-safe collection in .NET 4.0 which I was not aware of. Not sure used Bag or Collection for the situation. +1

[Vasyl Zvarydchuk]

ConcurrentBag doesn't implement IList and is not actually thread safe version of List

[John Demetriou]

What language are you talking about?

[Rusty Nail]

On very large List<T> Collections, Clone() should be avoided. Resource use should be thought about. Taking advantage of CODE Examples referencesource.microsoft.com/#mscorlib/system/Collections/… should be a priority.

[Cirelli94]

@denfromufa it look likes they added this in .net core 2.0 docs.microsoft.com/en-gb/dotnet/api/…

[jpmc26]

This answer has several problems: 1) ConcurrentDictionary is a dictionary, not a list. 2) It is not guaranteed to preserver order, as your own answer states, which contradicts your stated reason for posting an answer. 3) It links to a video without bringing the relevant quotes into this answer (which might not be in concordance with their licensing anyway).

[Nick]

Java? One of the few features I miss about it. But it's usually written as: Collections.synchronizedList(new ArrayList());

[Xaqron]

+1 Locking the whole collection (_root) in each access (read/write) makes this a slow solution. Maybe it is better for this class to remain internal.

[bytedev]

Why "reinvent the square wheel" when MS has now made many different types of Concurrent alternatives.

[user2163234]

This is valid C# assuming you have a using System.Collections or you could use var System.Collections.ArrayList.Synchronized(new System.Collections.ArrayList());

[Victor Yarema]

You can't rely on things like current implementation if it is not explicitly guaranteed by the documentation. Implementation may change any time without a notice.

[Raman Zhylich]

This implementation is not thread-safe. It still throws "System.InvalidOperationException: 'Collection was modified; enumeration operation may not execute.'"

[Siderite Zackwehdex]

That is not related to thread safety, but to the fact that you are iterating and changing the collection. The exception is thrown by the enumerator when it sees the list was changed. To get around this you need to implement your own IEnumerator or change the code so that it doesn't iterate and change the same collection at the same time.

[Don Cheadle]

@VasylZvarydchuk - sure technically it's not a List. But it's exactly what most people are looking for when they are using List but need something thread-safe.

[tytyryty]

@jpmc26, yes it is not a full replacement for List of course, however the same is with ConcurrentBag as an accepted answer - it is not strict replacement of List but work around. To reply your concerns: 1) ConcurrentDictionary is a dictionary not a list you are right, however list has array behind, which can index in O(1) same as dictionary with int as a key 2) yes order is not guaranteed by doc (even though it is preserved), but accepted ConcurrentBag cannot guarantee order as well in multithreaded scenarios

[Christoph]

That suggestion has potential in my opinion. If Dictionary.Count is used as a key (in case there are no deletes), any thread may add values like this while (!myDict.TryAdd(myDict.Count, myValue)) { } (or use an atomic increment of a counter in case there might be deletes). This would guarantee the values can be brought into the original order when retrieving them.

[Suncat2000]

It's not thread-safe because the collection can be changed during the "synchronized" methods. That absolutely is part of thread safety. Consider one thread calls Clear() after another calls this[index] but before the lock is activated. index is no longer safe to use and will throw an exception when it finally executes.

[Suncat2000]

Why should key be incriminated when it wasn't the key's fault?

[Richard II]

@Suncat2000 ha!

[Chris Rollins]

_i++ is not threadsafe. you have to use an atomic add when you increment it and probably mark it volatile too. CheckReset() is not threadsafe. Anything can happen in between the conditional check and call to Reset(). Don't write your own multithreading utilities.

[15ee8f99-57ff-4f92-890c-b56153]

@DonCheadle Except you can't remove items from it either. I've never yet found a case where it was useful.

[amarnath chatterjee]

ConcurrentBag is not a replacement of list. It doesn't behaves like list, You cannot remove elements like lists. In lists you can specify the item to be removed, you cannot do this with concurrent bags

[Ivan Ičin]

Unfortunately this isn't completely thread safe. Enumarations are not thread safe and that's one of the main reasons why one would pick this over other types.

[aruno]

I just broke my website using concurrentbag as a replacement to list. I thought it may have failed but I wasn’t sure. Would’ve been a lot easier if there was just a concurrent list like thing. I think I’ll try the stack next :)