How do you safely enumerate a List without finalizers getting in the way?

Question

I have a static list of WeakReference's in my application. At some point, I want to take a snapshot of all the currently "alive" objects in this list.

Code is like this:

private static readonly List<WeakReference> myObjects = new List<WeakReference>();

public static MyObject[] CollectObjects()
{
    var list = new List<MyObject>();
    foreach (var item in myObjects)
    {
        if (!item.IsAlive)
            continue;
        var obj = item.Target as MyObject;
        list.Add(obj);
    }
    return list.ToArray();
}

The problem I'm having is that I sometimes (rarely) get a "Collection Was Modified" exception in the foreach loop above. I only add/remove from this list in the MyObject constructor/finalizers, which looks like this:

public class MyObject
{
    private static readonly object _lockObject = new object();
    WeakReference referenceToThis;
    public MyObject()
    {
        lock (_lockObject)
        {
            referenceToThis = new WeakReference(this);
            myObjects.Add(referenceToThis);
        }
    }
    ~MyObject()
    {
        lock (_lockObject)
        {
            myObjects.Remove(referenceToThis);
        }
    }
}

Since nothing else in my code is touching the list, my assumption is therefore that the garbage collector is finalizing some of those objects just as I try to enumerate the list.

I thought about adding a lock (_lockObject) around the foreach loop but I'm not sure how such a lock would affect the GC?

Is there a better/correct/safer way to enumerate over a List of WeakReferences?

Have you tried to pass `true` as second parameter to `WeakReference` [ctor](https://learn.microsoft.com/en-us/dotnet/api/system.weakreference.-ctor?view=netframework-4.8#System_WeakReference__ctor_System_Object_System_Boolean_)? There is also an existing [thred](https://stackoverflow.com/questions/2837478/is-there-a-way-to-do-a-weaklist-or-weakcollection-like-weakreference-in-clr) which can be helpful — Pavel Anikhouski, Aug 05 '19 at 08:53
No, I have to admit that I don't understand what this parameter does. — Patrick Klug, Aug 05 '19 at 08:55
`~MyObject()` In C# , you dont know what timing call that , and if `myObjects `refrerence your object , It could not release for GC , So possible `~MyObject()` never worked ? — TimChang, Aug 05 '19 at 08:59
Not taking the lock around the foreach loop is a plain bug, you must fix it. Do note that it is generally not that sensible to be in a hurry to destroy a WeakReference. You might as well do it while you iterate the collection for example, albeit that you then need for instead of foreach. — Hans Passant, Aug 05 '19 at 10:48
Locking around insertion and removal but not around the foreach will clearly have no effect on your problem. But I'm not sure using lock here is a good idea anyhow : If you check for "aliveness" manually, why bother removing dead object from the list automatically? — XouDo, Feb 25 '21 at 11:47

Mike Nakis · Accepted Answer · 2021-02-25T11:29:18.233

A finalizer introduces a significant overhead in the entire garbage collection mechanism, so it is best to avoid it if you can. And in your case, you can avoid it and greatly simplify your design.

Instead of having your objects detecting their own finalization via finalizers and removing themselves from that list, replace your List<WeakReference<T>> with a WeakCollection<T>.

WeakCollection<T> is a collection, not a list. Never use a list there where a collection would suffice.

WeakCollection<T> fully encapsulates the fact that it contains weak references, meaning that you use it as a regular list, there is nothing about weak references in its interface.

WeakCollection<T> automatically removes weak references from itself when it detects that they have expired. (You will find that implementing it as a list is significantly more complicated than implementing it as a collection, so once again -- never use a list there where a collection would suffice.)

Here is a sample implementation of a WeakCollection<T>. Beware: I have not tested it.

namespace Whatever
{
    using Sys = System;
    using Legacy = System.Collections;
    using Collections = System.Collections.Generic;

    public class WeakCollection<T> : Collections.ICollection<T> where T : class
    {
        private readonly Collections.List<Sys.WeakReference<T>> list = new Collections.List<Sys.WeakReference<T>>();

        public void                           Add( T item )   => list.Add( new Sys.WeakReference<T>( item ) );
        public void                           Clear()         => list.Clear();
        public int                            Count           => list.Count;
        public bool                           IsReadOnly      => false;
        Legacy.IEnumerator Legacy.IEnumerable.GetEnumerator() => GetEnumerator();

        public bool Contains( T item )
        {
            foreach( var element in this )
                if( Equals( element, item ) )
                    return true;
            return false;
        }

        public void CopyTo( T[] array, int arrayIndex )
        {
            foreach( var element in this )
                array[arrayIndex++] = element;
        }

        public bool Remove( T item )
        {
            for( int i = 0; i < list.Count; i++ )
            {
                if( !list[i].TryGetTarget( out T target ) )
                    continue;
                if( Equals( target, item ) )
                {
                    list.RemoveAt( i );
                    return true;
                }
            }
            return false;
        }

        public Collections.IEnumerator<T> GetEnumerator()
        {
            for( int i = list.Count - 1; i >= 0; i-- )
            {
                if( !list[i].TryGetTarget( out T element ) )
                {
                    list.RemoveAt( i );
                    continue;
                }
                yield return element;
            }
        }
    }
}

Interesting, not sure how I feel about putting the maintenance logic in the GetEnumerator and having the GetEnumerator return the list in reverse order seems a bit odd to me. — Patrick Klug, Feb 26 '21 at 00:24
I repeat: stop thinking of it as a list. It is a collection. And as such, it is unordered. Therefore, items may be returned in any order. But if you really can't stand this, you can change it, it will just be slightly more complicated. As for the maintenance logic in the enumerator, well, that's how caches work, and this is essentially a cache. — Mike Nakis, Feb 26 '21 at 00:28
I think this is a much better way to do it, thanks again for sharing! — Patrick Klug, Mar 15 '21 at 23:45

AgentFire · Answer 2 · 2019-08-05T09:56:01.637

This may hurt the GC and/or finalizers (denending on the performance) because you are using the lock (Critical Section) mechanism in a fast-executing scenario.

You should indeed wrap the enumeration in a syncronization block, but you also should impletement SpinLock (how to) pattern (instead of the lock), which is great for fast sync blocks:

readonly SpinLock sl = new SpinLock();
bool lockTaken = false;

...

try
{
    sl.Enter(ref lockTaken);
    // do your thing
}
finally
{
    if (lockTaken)
    {
        sl.Exit();
    }
}

How do you safely enumerate a List without finalizers getting in the way?

2 Answers2