NotifyCollectionChangedEventArgs: Added item does not appear at given index (during removal)

Question

I implemented an ObservableStack, per this post: Observable Stack and Queue. It's working 99% of the time, but in some cases - rarely & seemingly without reason - when I try to pop the stack, I get an exception:

System.InvalidOperationException: Added item does not appear at given index '1'.
   at MS.Internal.Data.EnumerableCollectionView.ProcessCollectionChanged(NotifyCollectionChangedEventArgs args)
   at System.Windows.Data.CollectionView.OnCollectionChanged(Object sender, NotifyCollectionChangedEventArgs args)
   at System.Collections.Specialized.NotifyCollectionChangedEventHandler.Invoke(Object sender, NotifyCollectionChangedEventArgs e)
   ...

Here's the relevant code:

public class ObservableStack<T> : Stack<T>, INotifyCollectionChanged, INotifyPropertyChanged
{
    public virtual event NotifyCollectionChangedEventHandler CollectionChanged;

    public new virtual void Push(T item)
    {
        base.Push(item);
        var e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, base.Count-1);
        if (this.CollectionChanged != null)
            this.CollectionChanged(this, e);
    }

    public new virtual T Pop()
    {
        var item = base.Pop();
        var e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, base.Count);
        if (this.CollectionChanged != null)
            this.CollectionChanged(this, e);
        return item;
    }

    //...
}

In the rare cases it happens, the exception is thrown on the call to CollectionChanged() in Pop(). This is similar to the question INotifyCollectionChanged: Added item does not appear at given index '0' , but none of the answers seem applicable in this case. Note that replacing base.count with -1 or 0 on the Remove line, where the exception is being thrown, causes it to always fail. Likewise if I just exclude the index from the event args, it always complains "Collection Remove event must specify item position." I can technically make it work by changing the notification args to

var e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);

...But of course, that isn't actually correct, and I would like to get 'Remove' to report properly. None of the items being inserted have an overridden Equals() or operator=.

A little more background: the stack is being used in an UndoHistory, where when values are changed, the previous values are pushed to the stack, & popped when I undo. This works for the vast, vast majority of cases, which is why I'm having difficulty figuring out why the error is occurring in just a small number of cases. And unfortunately, I've been thus far unsuccessful in whittling it down to a small self-contained example that always shows the issue. For instance, in the real application, I might do an operation and undo it with no problem, but then do the same operation twice in a row, after which undoing will raise the exception. In a smaller/simplified example, I can't get it to happen at all. Note that the stack is also bound to an ItemsControl for displaying the undo history - though it's one-way only (i.e. the control isn't used for modifying the stack, only displaying its items).

Any ideas why it would throw such an exception in such seemingly random, rare cases would be greatly appreciated.

Answer 1

This error isn't being thrown by your code but by another class that is subscribing to your events. Probably something in WPF. This class is verifying that a copy of your stack collection that it holds somewhere is still valid, and finding that it is not.

The 99% likely cause is that there are changes to your stack that are not generating CollectionChanged events.

The first problem is that when Clear is called you should issue a Reset event.

The second is that if someone casts your object to Stack<T> and calls Push or Pull, your code won't be hit. This happens because those methods aren't virtual, hence your need to declare your own versions with new. Declaring your methods as virtual does not change this.

You can't fix that. The solution is not to derive your class from Stack but to instead create a private Stack object inside it, and to then create your own methods that expose its functionality in a way that you are in complete control of.

Once you do this (and implement all events correctly!) the exceptions will go away.

Answer 2

Both your posted NotifyCollectionChangedEventArgs are created with wrong args.
A Stack is like a reversed collection: the last item added is the first to be removed and has always the index 0. So in a Stack the change always takes place at index 0.
Currently you are always reporting the last index, which is count - 1, which is the last item to pop, which is the first item pushed, which only points to the item that is currently popped when count == 1.

NotifyCollectionChangedEventArgs with fixed change index:

public new virtual void Push(T item)
{
    base.Push(item);
    var e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, 0);
    if (this.CollectionChanged != null)
        this.CollectionChanged(this, e);
}

public new virtual T Pop()
{
    var item = base.Pop();
    var e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item, 0);
    if (this.CollectionChanged != null)
        this.CollectionChanged(this, e);
    return item;
}

Remarks

The point is, that when a collection is used with an ItemsControl, the control will only operate on the ICollectionView of the source collection. This CollectionView internally subscribes to the underlying collection's CollectionChanged event to track the changes. It does this because the collection view maintains a pointer, which includes CurrentItem and CurrentPosition.
In this context CollectionView validates the changed item and its index by tracking those info from add to remove. When adding an item, you currently report the last index e.g. 1, but when removing this item, the true index is 0. That's why the internal validation fails and the exception "Added item does not appear at given index '1'." is thrown.

Note that a data structure like a stack (or like a queue), is not considered index based. But the collection change notifications are.

To get the expected behavior, you should extend an index based collection like ObservableCollection. Then use Insert at 0 and RemoveAt(0) to provide the LIFO behavior.

If you want O(1) for access operations, you would have to implement e.g., ICollection<T> + INotifyCollectionChanged and use an array as backing store.
Always append to the array and remember the last index. But take care that when implementing the indexer or GetEnumerator, that you always return the "reversed" array: first added item has the index 0. This way the UI gets the properly ordered collection view and you have your O(1) for push and pop operations.
The trick is to find a good initial array size to avoid resizing.

A stack is a data structure that only cares to return the last added item. You cannot move items or access items randomly (by index). That's why a Stack or Queue is not suited for the UI scenario. ItemsControl (and CollectionChanged) always operates index based.