2
\$\begingroup\$

I have a long running operation that I need to wait for or timeout. I have working code, but I would like your opinion on improving it.

public bool RegisterEquipmentListUpdate(int ID)
{
    if (Authentication.CheckSession(Session))
    {
        var wcfmonitoring = new WCFMonitoring.MonitoringDatabaseClient();
        try
        {
            bool timeout = false;
            DateTime start = DateTime.Now;
            //loop as long as the time isn't reached (600 seconds)
            while (!timeout)
            {
                if (wcfmonitoring.CheckForEquipmentUpdate(Authentication.GetSessionID(Session), Authentication.GetPTO(Session), ID))
                {
                    wcfmonitoring.Close();
                    //sleep before returning true, this is set so the importer can finish multiple files without the client refreshing on the first one
                    Thread.Sleep(2000);
                    return true;
                }
                else
                {
                    //sleep for 10 seconds before trying again
                    Thread.Sleep(10000);
                    //if the elapsed time is more than 10 minutes return false
                    if (start < DateTime.Now.AddSeconds(-600))
                    {
                        wcfmonitoring.Close();
                        return false;
                    }
                }
            }
        }
        catch
        {
            wcfmonitoring.Abort();
        }
    }
    //return false in case of error
    return false;
}
\$\endgroup\$
  • \$\begingroup\$ have you considered async/await \$\endgroup\$ – Nkosi Mar 2 '18 at 12:45
  • \$\begingroup\$ Ok, good idea. I can return async Task<bool> and await Task.Delay(2000) and await Task.Delay(10000). \$\endgroup\$ – Raskolnikov Mar 2 '18 at 13:16
  • \$\begingroup\$ Yes which would be a lot cleaner than blocking thread \$\endgroup\$ – Nkosi Mar 2 '18 at 13:19
  • \$\begingroup\$ You could also consider changing the approach and using a timer as apposed to a busy wait....just thinking aloud here \$\endgroup\$ – Nkosi Mar 2 '18 at 13:44
1
\$\begingroup\$

This is a rough draft, since I do not know the overall picture of your project. Some design might be improved to not even use this code (events, semaphores, mutexes, and other more predictable synchronization mechanisms). I rarely use this pattern for code that I control, more for code I cannot that I know is blocking. This approach is risky, as it can lead to undesired state and data loss. You have been warned! (http://thedailywtf.com/articles/My-Tales) for reference!)

If you stay on this path, I recommend A separate class/method be responsible for this sort of behaviour, to favour reuse, this also includes the possibility of having a CancellationToken passed in, just in case you change your mind about the request:

public async Task<T> WaitForActionCompletionOrTimeout<T>(Func<T> action, int timeout, CancellationToken? globalCancellation = null)
{
   var result = default(T); //return default for error

   var localCancellation = new CancellationTokenSource();
   var localAndGlobalCancellation = 
            CancellationTokenSource.CreateLinkedTokenSource(localCancellation.Token, 
                                                            globalCancellation.HasValue ? 
                                                            globalCancellation.Value : 
                                                            CancellationToken.None);

   Task cancelationTask = null;
   Task actionTask = null;

   try
   {
     cancelationTask = Task.Delay(timeout, localAndGlobalCancellation);
     actionTask = Task.Factory.StartNew(() =>
     {
       result = action.Invoke();
     }, localAndGlobalCancellation);
   }
   catch (Exception ex)
   {
     Trace.WriteLine(ex.Message);
     localCancellation.Cancel();
     return null;
   }

   await Task.WhenAny(actionTask, cancelationTask).ContinueWith(t => localCancellation.Cancel());
   return result;
}

Change the work method to return true or false, something like:

public bool CheckForUpdate()
 {
     using (var wcfmonitoring = new WCFMonitoring.MonitoringDatabaseClient())
     {
          var sw = new SpinWait();
          while (!wcfmonitoring.CheckForEquipmentUpdate(Authentication.GetSessionID(Session), Authentication.GetPTO(Session), ID)) //maybe  something shorter would work :) some variables are nice for debugging
          {
              sw.SpinOnce();  //magic Thread.Sleep(5) replacement with lots of goodies :) nice class to look at
          }

          Thread.Sleep(2000); //for the importer
          return true;
    }
}

And then:

var success = WaitForActionCompletionOrTimeout(() => CheckForUpdate, 600000 ); //I would so like 6.minutes.in.miliseconds.... ah dreams

This sort of approach (cancelling the worker) could also be done with threads in a much shorter and brute-forcier way for a slow performance increase in case of very low timeouts.

I would also advise using using and if WcfMonitoring is not IDisposable, to make it IDisposable.

One more question: What is the intent/difference between calling Abort and Close in your code?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.