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