Updating an ObservableCollection<T> asynchronously results in hangs, and no GUI update

Question

I'm implementing a visual version of Tracert (as a learning exercise) in WPF where results go to a listbox. The issues are (1) the listbox bound to tracertDataView is not updating, but (2) my entire application hangs.

I'm sure #2 is a threading issue but I'm not sure how to correct it (in the right way). In addition I'm not sure my technique of updating / binding the results of "DoTrace" are correct.

Here is my datasource in App.xaml

<Window.Resources>
<CollectionViewSource 
          Source="{Binding Source={x:Static Application.Current}, Path=TracertResultNodes}"   
          x:Key="tracertDataView" />

</Window.Resources>

App.xaml.cs

public partial class App : Application
{
    private ObservableCollection<TracertNode> tracertResultNodes = new ObservableCollection<TracertNode>();

    public void AppStartup(object sender, StartupEventArgs e)
    {
          // NOTE: Load sample data does work correctly.. and displays on the screen.  
         //      subsequent updates do not display
        LoadSampleData();
    }

    private void LoadSampleData()
    {

         TracertResultNodes = new ObservableCollection<TracertNode>();

        TracertNode t = new TracertNode();
        t.Address = new System.Net.IPAddress(0x2414188f);
        t.RoundTripTime = 30;
        t.Status = System.Net.NetworkInformation.IPStatus.BadRoute;

            TracertResultNodes.Add(t);
    }

    public ObservableCollection<TracertNode> TracertResultNodes
    {
        get { return this.tracertResultNodes; }
        set { this.tracertResultNodes = value; }
    }
}

Here is the MainWindow code

  public partial class MainWindow : Window
{
    CollectionViewSource tracertDataView;
    TraceWrapper _tracertWrapper = null;

    public MainWindow()
    {
        InitializeComponent();
         _tracertWrapper = new TraceWrapper();

        tracertDataView = (CollectionViewSource)(this.Resources["tracertDataView"]);
    }

    private void DoTrace_Click(object sender, RoutedEventArgs e)
    {
       ((App)Application.Current).TracertResultNodes = _tracertWrapper.Results;

       _tracertWrapper.DoTrace("8.8.8.8", 30, 50);
    }
}

FYI Internal implementation Detail of instance object "traceWrapper.DoTrace"

    /// <summary>
    /// Trace a host.  Note that this object internally calls the Async implementation of .NET's PING. 
    // It works perfectly fine in a CMD host, but not in WPF
    /// </summary>
     public ObservableCollection<TracertNode> DoTrace(string HostOrIP, int maxHops, int TimeOut)
    {
        tracert = new Tracert();

        // The following is triggered for every host that is found, or upon timeout
         //  (up to 30 times by default)
        AutoResetEvent wait = new AutoResetEvent(false);
       tracert.waiter = wait;

        tracert.HostNameOrAddress = HostOrIP;

        tracert.Trace();

        this.Results = tracert.NodeList;

        while (tracert.IsDone == false)
        {
            wait.WaitOne();
            IsDone = tracert.IsDone;
        }
        return tracert.NodeList;
    }

Answer 1

I don't understand how u used AutoResetEvent, i guess it is not supposed to be used in this way :)

But since Trace run already in another thread, are you sure there is not an event "OnTracertComplete" or something like that in your Tracert class?

If there is not, why you just don't put a DispatchTimer into your application? That timer would periodically poll until tracert.IsDone becomes true. If you block the execution of the application thread until an operation completes, you block the execution of the window event loop so window will never be updated.

Another important thing: you cannot update ObservableCollections from another thread. Be careful and be sure that everything that is updated in the WPF window is executed from the same thread of the window. Don't know what your Trace class do exactly, but your problem here seems to be of course the wait loop, that don't makes sense in a GUI application.

Use notification events or a timer to poll the result. A timer with 1 second resolution seems good to me for this particular implementation and the performance inpact is absolutely minimal.

This is a possible implementation if you are able to modify the Tracert class.

    public delegate void TracertCallbacHandler(Tracert sender, TracertNode newNode);

    public class Tracert
    {
        public event TracertCallbacHandler NewNodeFound;
        public event EventHandler TracertCompleted;

        public void Trace()
        {
            ....
        }

        // This function gets called in tracert thread\async method.
        private void FunctionCalledInThreadWhenPingCompletes(TracertNode newNode)
        {
            var handler = this.NewNodeFound;
            if (handler != null)
                handler(this, newNode);
        }

        // This function gets called in tracert thread\async methods when everything ends.
        private void FunctionCalledWhenEverythingDone()
        {
            var handler = this.TracertCompleted;
            if (handler != null)
                handler(this, EventArgs.Empty);
        }

    }

And here is the code to run the tracert, This is TracertWrapper.

    // Keep the observable collection as a field.
    private ObservableCollection<TracertNode> pTracertNodes;

    // Keep the instance of the running tracert as a field, we need it.
    private Tracert pTracert;

    public bool IsTracertRunning
    {
        get { return this.pTracert != null; }
    }

    public ObservableCollection<TracertNode> DoTrace(string hostOrIP, int maxHops, int timeOut)
    {
        // If we are not already running a tracert...
        if (this.pTracert == null)
        {
            // Clear or creates the list of tracert nodes.
            if (this.pTracertNodes == null)
                this.pTracertNodes = new ObservableCollection<TracertNode>();
            else
                this.pTracertNodes.Clear();

            var tracert = new Tracert();
            tracert.HostNameOrAddress = hostOrIP;
            tracert.MaxHops = maxHops;
            tracert.TimeOut = timeOut;

            tracert.NewNodeFound += delegate(Tracert sender, TracertNode newNode)
            {
                // This method is called inside Tracert thread.
                // We need to use synchronization context to execute this method in our main window thread.

                SynchronizationContext.Current.Post(delegate(object state)
                {
                    // This method is called inside window thread.
                    this.OnTracertNodeFound(this.pTracertNodes, newNode);
                }, null);
            };

            tracert.TracertCompleted += delegate(object sender, EventArgs e)
            {
                // This method is called inside Tracert thread.
                // We need to use synchronization context to execute this method in our main window thread.

                SynchronizationContext.Current.Post(delegate(object state)
                {
                    // This method is called inside window thread.
                    this.OnTracertCompleted();
                }, null);
            };

            tracert.Trace();

            this.pTracert = tracert;
        }

        return this.pTracertNodes;
    }

    protected virtual void OnTracertCompleted()
    {
        // Remove tracert object,
        // we need this to let the garbage collector being able to release that objects.
        // We need also to allow another traceroute since the previous one completed.
        this.pTracert = null;

        System.Windows.MessageBox.Show("TraceRoute completed!");
    }

    protected virtual void OnTracertNodeFound(ObservableCollection<TracertNode> collection, TracertNode newNode)
    {
        // Add our tracert node.
        collection.Add(newNode);
    }

Answer 2

The issue is that not only is the listbox not updating, but my entire application hangs.

This is probably due to the AutoResetEvent blocking in DoTrace. You explicitly call Wait.WaitOne(); on the event handle, but as far as I can tell, never Set() it. This will cause the application to hang forever as soon as you call Wait.WaitOne().

It sounds like tracert.Trace() is an asynchronous method. Does it include some form of callback/event to notify you upon completion? If so, you should use that, not poll in a loop, to determine when it's complete.

Answer 3

(1) the listbox bound to tracertDataView is not updating

You won't see the updates to your listbox, as you're assigning a new collection to the TracertResultNodes property, the binding in this case simply does not work, because a new collection was assigned.

In addition to ensuring that the collection is updated in the same thread as outlined by Salvatore below, you should only add or remove items from the existing collection, and NOT assign the new one generated by your DoTrace function.

private void DoTrace_Click(object sender, RoutedEventArgs e)
    {
       foreach(var traceNode in _tracertWrapper.Results)
       {
          ((App)Application.Current).TracertResultNodes.Add(traceNode);
       }

       _tracertWrapper.DoTrace("8.8.8.8", 30, 50);
    }

If you do assign a new one, then you'd need to implement INotifyPropertyChanged on your App class, am not sure how (or whether) that would work though (I have not tried this before).