Is .NET ObservableCollection<> ToList() thread safe? If not, how to proceed

Question

I have an ObservableCollection of Logs that I have bound to my GUI through a property

public ObservableCollection<ILog> Logs {get; private set;}

There is a requirement to show a subset of the logs somewhere else so I have:

public ObservableCollection<ILog> LogsForDisplay
    {
        get
        {
            ObservableCollection<ILog> displayLogs = new ObservableCollection<ILog>();
            foreach (Log log in Logs.ToList()) // notice the ToList()
            {
                if (log.Date != DateTime.Now.Day)
                    continue;
                displayLogs.Add(log);
            }
            return displayLogs;

        }

Before I added the "ToList()" I got exceptions occasionally about "Collection was modified; enumeration operation may not execute" Makes sense - somebody could add to Logs while I'm iterating over it. I got the idea of "ToList" from Collection was modified; enumeration operation may not execute which seems to suggest that ToList is the way to go and implies it's thread safe. But is ToList() thread safe? I assume that internally it must use the list and iterate over it? And what if someone adds to that list at the same time? Just because I haven't seen a problem doesn't mean there isn't one.

My question. Is ToList() thread safe and if not, what is the best pattern for protecting Logs? If ToList() is thread safe, do you have a reference?

Bonus question. If the requirements were to change and all I needed to display on the GUI was LogsForDisplay and NOT Logs, could I change Logs to something else that would solve the problem? Such as ImmutableList ? Then I wouldn't have to call ToList<> which I assume takes some time to make a copy.

Let me know if I can provide clarification. Thanks,

Dave

The [documentation](https://msdn.microsoft.com/en-us/library/ms668604%28v=vs.110%29.aspx) doesn't guarantee any useful method or property to be thread safe, thus you have to assume they all aren't. You could go for the `lock` in you logging method (where you are adding to `Logs` collection) and around `ToList` in question. This seems like only sure way to deal with this. Possibly override all insertion etc. methods in your class derived from `ObservableCollection` to lock using [`SyncRoot`](https://msdn.microsoft.com/en-us/library/bb353794(v=vs.110).aspx) property. — slawekwin, Mar 20 '17 at 09:43
All good answers and I thank everyone! Can only pick 1 unfortunately. — Dave, Mar 27 '17 at 18:22

score 7 · Accepted Answer · answered Mar 20 '17 at 15:20

The implementation of ToList extension method boils down to copying items from one array to another via Array.Copy method, which, while hiding Collection was modified error from you is not thread-safe and you may face weird behavior when underlying items are changed during Array.Copy calll.

What I'd suggest it to use CollectionView for binding, I've been using it for quite a long time in similar cases and faced no issues so far.

// somewhere in .ctor or other init-code
var logsForDisplay = new CollectionView(this.Logs);
logsForDisplay.Predicate = log => ((Log)log).Date == DateTime.Now.Day;

public CollectionView LogsForDisplay { get { return this.logsForDisplay; } }

You can have another CollectionView for different use case, e.g.:

// somewhere in .ctor or other init-code
var yesterdaysLogs = new CollectionView(this.Logs);
yesterdaysLogs.Predicate = log => ((Log)log).Date == DateTime.Now.AddDays(-1).Day;

public CollectionView YesterdaysLogs{ get { return this.yesterdaysLogs; } }

score 3 · Answer 2 · answered Mar 26 '17 at 07:21

The simple solution is to implement your own thread safe ObservableCollection, A simple thread friendly version of the observable collection:

 public class NEWObservableCollection<T> : ObservableCollection<T>
    {
        public override event NotifyCollectionChangedEventHandler CollectionChanged;
        protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
        {
            NotifyCollectionChangedEventHandler CollectionChanged = this.CollectionChanged;
            if (CollectionChanged != null)
                foreach (NotifyCollectionChangedEventHandler notifyCollectionChangedEventHandler in CollectionChanged.GetInvocationList())
                {
                    DispatcherObject dispatcherObject = notifyCollectionChangedEventHandler.Target as DispatcherObject;
                    if (dispatcherObject != null)
                    {
                        Dispatcher dispatcher = dispatcherObject.Dispatcher;
                        if (dispatcher != null && !dispatcher.CheckAccess())
                        {
                            dispatcher.BeginInvoke(
                                (Action)(() => notifyCollectionChangedEventHandler.Invoke(this,
                                    new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset))),
                                DispatcherPriority.DataBind);
                            continue;
                        }
                    }
                    notifyCollectionChangedEventHandler.Invoke(this, e);
                }
        }
    }

score 1 · Answer 3 · answered Mar 20 '17 at 17:33

The ToList extension method is "thread-safe" when the following two conditions are satisfied:

The Logs collection is only modified using the Add method. That is, only by adding items to the end of the collection. Items are never removed or inserted. This guarantees that iterating over the items is safe.
One of the following two conditions is satisfied:
- Existing items are never modified.
- Existing items might be modified but the latest (consistent) state of all items in the collection is not required in the LogsForDisplay.get method.

If these conditions are not satisfied, you'll have to either use ImmutableList as the underlying collection of ObservableCollection or use locks.

If these two conditions are satisfied, you don't have to use foreach and make a copy of the collection using ToList<TSource>, you can safely use a for loop with indexing.

Sotiris · Answer 4 · 2017-03-24T09:10:19.550

As alex.b said, the .ToList method is not thread Safe and this link to the source code proves that http://referencesource.microsoft.com/#mscorlib/system/collections/generic/list.cs,d2ac2c19c9cf1d44

What are your options? Well, you have many:

If you want to stick around with the ObservableCollection and be 100% safe then you have to wrap the Logs.ToList() with a lock statement.Of course in that case you need wrap with a lock any process that modifies the Logs Collection.
I noticed that the LogsForDisplay is a a readonly(?) property possibly bound to a WPF grid. If you want to display the data only on demand and not every time the Log collection changes, then you can easily replace the type of the Logs with a thread safe collection such as an Immutable collection or a collection from System.Collections.Concurrent namespace like the ConcurrentDictionary. Because you want to return a subset of the Logs you cannot avoid copying the items into another list and return it later. Even in this case, you still have to use the Lock when call the .ToList() extension, but only once.

score 1 · Answer 5 · answered Mar 24 '17 at 16:10

The ToList is not thread safe and unlikely could be thread safe since its an extension method. This means it could provide thread safety only within some set of extension methods which all would use some synchronization, but that wouldn't protect from direct thread unsafe calls on the collection. See the implementation here (as was already referenced).

But why are you talking about thread safety at all? The ObservableCollection is not thread safe itself, so its strange you're saying some concurrent operations could be the source of the original error. Thus, if you used the Logs collection correctly, you wouldn't have to use ToList at all.

Andrii Litvinov · Answer 6 · 2017-03-26T12:48:53.077

Enumerator.MoveNext method throws an exception when collection is modified after you started enumerating it in forreach loop. Method ToList will copy internal array of ObservableCollection because source collection implements ICollection<T> interface and won't throw such exception because the size of the array cannot be changed. If the ObservableCollection is modified meanwhile you won't get those changes. But the data you are going to return is stale anyway. So it is safe approach and good enough in your case.

Links so that you can check and ensure yourself: Enumerable.cs and List.cs

Solution provided by @alex.b will also work well, if it meets your requirements.

You don't need any thread safe collections for this task because they will only add extra synchronization overhead.

Is .NET ObservableCollection<> ToList() thread safe? If not, how to proceed

6 Answers6