Timer OnTimedEvent causing UI to become unresponsive but should be running on another thread

Question

I have a timer, that triggers a timed event that iterates through a TreeView and pings all the IP addresses associated with each node.

I want this to be done in the background on a separate thread. I (probably wrongly) thought that an OnTimedEvent exists on a separate thread to the UI.

However, when my program starts, it is responsive. Then as soon as the timed event triggers the UI becomes unresponsive, doesn't crash but doesn't regain responsiveness until the program is stopped in the debugger and ran again.

I've been messing around with async and await in order to get the required behaviour however I am unable to achieve this.

Here's how I initialise the Timer currently.

public Form1()
{
   InitializeComponent();
   StartTimer();
}

private async void StartTimer()
{
   aTimer = new System.Timers.Timer(15000);
   await Task.Run(() => aTimer.Elapsed += OnTimedEvent);
   aTimer.AutoReset = true;
   aTimer.Enabled = true;
   aTimer.SynchronizingObject = this;
}

And the OnEventTimed method

private async void OnTimedEvent(Object source, ElapsedEventArgs e)
{
   // Iterate through all root nodes
   foreach (TreeNode tn in mainTree.Nodes)
   {
      TreeNodeCollection rootNodes = ((TreeNode)tn).Nodes;

      foreach (TreeNode node in rootNodes)
      {
         // Create ping object
         System.Net.NetworkInformation.Ping pinger = new();
         PingReply pingReply;

         if (node.Tag != null)
         {
             pingReply = pinger.Send(node.Tag.ToString());

             if (pingReply.Status.ToString().Contains("Success"))
             {
                UpdateUI(node, 1); // If successful, set the image index to show green 
             }
             else if (pingReply.Status.ToString().Contains("Failed"))
             {
                UpdateUI(node, 2);
             }
          }
       }


       // Iterate through all the children of the 'root' nodes
       foreach (TreeNode child in tn.Nodes)
       {
           // Extract all nodes from these children
           TreeNodeCollection myNodes = ((TreeNode)child).Nodes;

           // Create ping object
           System.Net.NetworkInformation.Ping pinger = new();
           PingReply pingReply;

           // Iterate through each of the nodes, send a ping request and then update the UI based on the result of the ping
           foreach (TreeNode node in myNodes)
           {
              if (node.Tag != null)
              {
                  pingReply = pinger.Send(node.Tag.ToString());

                  if (pingReply.Status.ToString().Contains("Success"))
                  {
                     UpdateUI(node, 1); // If successful, set the image index to show green 
                  }
                  else if (pingReply.Status.ToString().Contains("Failed"))
                  {
                     UpdateUI(node, 2);
                  }
               }
           }
       }

   }
}

I am trying to use await Task.Run(() => aTimer.Elapsed += OnTimedEvent); but it's not giving me the behaviour I expect any help would be appreciated

What's the intention behind the line `aTimer.SynchronizingObject = this;`? — Theodor Zoulias, Oct 28 '22 at 14:45
@TheodorZoulias allows me to update UI outside of the timer thread. I got it from this question I asked: https://stackoverflow.com/questions/73963432/accessing-ui-thread-while-using-system-timer-and-treeview — Thomton, Oct 28 '22 at 14:46
The effect of this line is that the event handler is invoked on the UI thread. — Theodor Zoulias, Oct 28 '22 at 14:47
From a casual glance it appears you are just assigning an event handler asynchronously. Usually, the background task would be assigned when the event handler function fires. — Ross Bush, Oct 28 '22 at 14:53
What is the desirable behavior regarding updating the UI? Do you want the color of the nodes to be updated one by one, or to update all of them at once when the pinging of all IPs has completed? Also do you want to ping the IPs sequentially (the one after the other), or multiple IPs concurrently? One more question, what is the target .NET platform? .NET 6? — Theodor Zoulias, Oct 28 '22 at 14:58
@TheodorZoulias Updating the related node once a ping response is received, I'm not too bothered about them all being updated at the same time or even pinged at the same time, as the outcome 99% of the time will be they will stay green and will only change from green rarely. And yes .NET 6. — Thomton, Oct 28 '22 at 15:05

Theodor Zoulias · Accepted Answer · 2022-10-29T00:08:01.990

My suggestion is to ditch the mischievous System.Timers.Timer, and use instead a normal System.Windows.Forms.Timer. Then in the Tick event handler just offload to the ThreadPool any work that you don't want to happen on the UI thread. All interaction with UI controls should stay on the UI thread. Here is an (untested) example:

private async void timer_Tick(object sender, EventArgs e)
{
    timer.Enabled = false;

    //...

    if (node.Tag != null)
    {
        string ip = node.Tag.ToString();

        PingReply pingReply = await Task.Run(() => pinger.Send(ip));

        if (pingReply.Status == IPStatus.Success)
            node.ImageIndex = 1;
        else
            node.ImageIndex = 2;
    }
        
    //...

    timer.Enabled = true;
}

The Task.Run is the method that does the offloading. The action of the Task.Run is invoked on the ThreadPool. The result of the invocation is marshaled back on the UI thread, by awaiting the resulting Task<PingReply>.

Notice how the Task.Run delegate is not touching at the slightest the TreeNode UI elements. It gets an argument of type string, and returns a result of PingReply. All the UI-related work is performed exclusively on the UI thread.

Timer OnTimedEvent causing UI to become unresponsive but should be running on another thread

1 Answers1