ReadOnlyObservableCollection anti-pattern

I recently fixed a bug that was a result of a subtle misuse of the ReadOnlyObservableCollection<T> class. The code in question was structured as follows: The Source class has a collection of Items. Clients should be able to observe but not modify the collection, so it’s exposed as a ReadOnlyObservableCollection. The Client class adds an event handler to this collection, and removes it at the appropriate time (to prevent a possible memory leak).

class Source
{
    Source()
    {
        m_collection = new ObservableCollection<int>();
    }

    public ReadOnlyObservableCollection<int> Items
    {
        get
        {
            // this is the bug

            return new ReadOnlyObservableCollection<int>(m_collection);
        }
    }

    readonly ObservableCollection<int> m_collection;
}

class Client
{
    // obtain Source instance from somewhere

    Source m_source;
    void Subscribe()
    {
        ((INotifyCollectionChanged) m_source.Items).CollectionChanged += SourceItems_CollectionChanged;
    }

    void Unsubscribe()
    {
        ((INotifyCollectionChanged) m_source.Items).CollectionChanged += SourceItems_CollectionChanged;
    }

    void SourceItems_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) { }
}

The bug is that the Source.Items property creates a new ReadOnlyObservableCollection instance each time it’s accessed, which means that the calls to add and remove the event handler happen on two different objects; the event handler is never actually removed.

One solution is for the Client class to cache the exact INotifyCollectionChanged object it subscribed to, so it can be sure to unsubscribe from the same object. The other (and better) solution is for the Source class to create and hold a ReadOnlyObservableCollection, and return the same object each time the Items property is accessed:

public class Source
{
    Source()
    {
        m_collection = new ObservableCollection<int>();
        m_collectionReadOnly = new ReadOnlyObservableCollection<int>(m_collection);
    }

    public ReadOnlyObservableCollection<int> Items
    {
        get { return m_collectionReadOnly; }
    }
    
    readonly ObservableCollection<int> m_collection;
    readonly ReadOnlyObservableCollection<int> m_collectionReadOnly;
}

This solves the initial bug and also has the benefit of allowing all clients to share the same ReadOnlyObservableCollection instance (instead of creating one per client).

Posted by Bradley Grainger on March 17, 2009