Treeview Async Problems

Question

I am having a problem with a treeview where I try to add a subnode based upon the Node.Text Index (I have also tried this based upon the int index - to no avail). This works wonderfully when ran synchronously. However I run the exact same thing Async (backgroundWorker) it throws an unhandled ArgumentOutOfRange Exception. Another odd part is that I've attempted to catch this Exception in two different areas. see code:

 using (Microsoft.Win32.RegistryKey key = Registry.LocalMachine.OpenSubKey(registry_key))
        {
            int x = 0;
            foreach (string subkey_name in key.GetSubKeyNames())
            {
                using (RegistryKey subkey = key.OpenSubKey(subkey_name))
                {
                    foreach (string a in (string[])subkey.GetValue("Users", ""))
                    {
                        User u = new User(a);
                        usrs.addUser(new User(a));
                        wgs.addUserToWorkgroup(subkey_name, a);
                        usrs.AddWorkGroupToUser(subkey_name, a);
                        int trycount = 0;
                    TryAgain:
                        try
                        {
                            //here is where the exception occurs
                            ExecuteSecure(() => treeView1.Nodes[subkey_name].Nodes.Add(a, a));
                        }
                        catch (ArgumentOutOfRangeException)//This does not catch it.
                        {
                            trycount++;
                            if (trycount < 100)
                            {
                                goto TryAgain; //b/c I cannot catch it this never happens...
                            }
                        }

                    }
                }

                x++;
                //System.Threading.Thread.Sleep(2);
                //As you can see I've tried to let the tread sleep to resolve this 
                //- it will get a little farther but still eventually bomb out.
            }
        }

Here is the ExecuteSecure code (https://stackoverflow.com/a/8021020/1387186)

 private void ExecuteSecure(Action a)
    {
        o = new object();
        try
        {
            if (InvokeRequired)
            {
                lock (o)
                {
                    BeginInvoke(a);
                }
            }
            else
                a();
        }
        catch (Exception) //again **sigh** this does not catch the error
        { }

Another victim of closing over the for loop variable. http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx — Hans Passant, May 25 '12 at 13:34
possible duplicate of [Closing over the Loop Variable in C#](http://stackoverflow.com/questions/9626051/closing-over-the-loop-variable-in-c-sharp) — Hans Passant, May 25 '12 at 13:35
It seems like an attempt to prevent concurrent action execution. It didn't work because you use a local object to lock. — Viacheslav Smityukh, May 25 '12 at 13:50
your lock(o) before `BeginInvoke` is pointless, o is a new value (i.e. nothing else could ever use it to lock) *and* `BeginInvoke` returns immediately... which is why you don't catch anything. — Peter Ritchie, May 25 '12 at 13:59
Yeah the lock isnt doing anything I get that - I just havent removed it or changed its scope — Wjdavis5, May 25 '12 at 14:29

score 2 · Accepted Answer · answered May 25 '12 at 17:24

You have several problems.

You are closing over the loop variable.
Your lock is pointless because you use a different instance each time.
Your lock is pointless because no other threads compete for it.
Your new solution of using Invoke cause the UI and worker threads to ping-pong back and forth and as such your worker thread now became useless.

Anyone who monitors my answers on here knows how I feel about these marshaling techniques (like Invoke). It can be (and usually is) the worst method of updating the UI. In fact, it is so overused that it has gotten to the point where I think it can be considered a form of cargo cult programming.

What you should do is have your worker thread publish the data required to update the TreeView into a queue and then have your UI thread pull it out via a System.Windows.Forms.Timer on an interval that works best for you. Here is what it would look like.

public class YourForm : Form
{
  private sealed class YourData
  {
    public string SubKeyName { get; set; }
    public string Value { get; set; }
  }

  private ConcurrentQueue<YourData> queue = new ConcurrentQueue<YourData>();

  private void StartTreeViewUpdate_Click(object sender, EventArgs args)
  {
    Task.Factory.StartNew(WorkerThread);
    TreeViewUpdateTimer.Enabled = true;
  }

  private void TreeViewUpdateTimer_Tick(object sender, EventArgs args)
  {
    // Update in batches of 100 (or whatever) so that the UI stays
    // responsive.
    treeView1.BeginUpdate();
    for (int i = 0; i < 100; i++)
    {
      YourData value = null;
      if (queue.TryDequeue(value) && value != null)
      {
        treeView1.Nodes[value.SubKeyName].Nodes.Add(value.Value, value.Value)
      }
      else
      {
        // We're done.
        TreeViewUpdateTimer.Enabled = false;
        break;
      }
    }
    treeView1.EndUpdate();
  }

  private void WorkerThread()
  {
    using (RegistryKey key = Registry.LocalMachine.OpenSubKey(registry_key))
    {
      foreach (string subkey_name in key.GetSubKeyNames())
      {
        using (RegistryKey subkey = key.OpenSubKey(subkey_name))
        {
          foreach (string a in (string[])subkey.GetValue("Users", ""))
          {
            User u = new User(a);
            usrs.addUser(new User(a));
            wgs.addUserToWorkgroup(subkey_name, a);
            usrs.AddWorkGroupToUser(subkey_name, a);
            var data = new YourData();
            data.SubKeyName = subkey_name;
            data.Value = a;
            queue.Enqueue(data);
          }
          queue.Enqueue(null); // Indicate that queueing is done.
        }
      }
    }
  }
}

I dont believe I actually am closing over the variable here, I say that based upon what I've read here: ["can is newly created every "lap" around the loop, never changes, and won't cause problems"](http://stackoverflow.com/a/9626173/1387186) Nonetheless your idea seems the best choice. I've been bugging myself to come up with a messaging system that I can reuse and learn with, but I just havent prioritized it. Perhaps you'll be my muse to do things the correct way. Could you offer any good jumping off points? — Wjdavis5, May 29 '12 at 01:43

score 0 · Answer 2 · answered May 25 '12 at 13:57

In case the comments are obvious, do this:

 var uncapturedSubKey_name = subkey_name;
 ExecuteSecure(() => treeView1.Nodes[uncapturedSubKey_name].Nodes.Add(a, a));

The lambda is capturing the the other variable and isn't executing until after the loop is done (i.e. the last subkey_name...

score 0 · Answer 3 · answered May 25 '12 at 14:30

I was able to resolve this like so:

By changing the BeginInvoke() to Invoke() I also changed the scope of O so it actually locks.

private void ExecuteSecure(Action a)
{

    try
    {
        if (InvokeRequired)
        {
            lock (o)
            {
                Invoke(a);
            }
        }
        else
            a();
    }
    catch (Exception) //again **sigh** this does not catch the error
    { }

Treeview Async Problems

3 Answers3