Long running operation with timeout

Question

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;
}

• Nkosi: have you considered async/await
• Raskolnikov: Ok, good idea. I can return async Task<bool> and await Task.Delay(2000) and await Task.Delay(10000).
• Nkosi: Yes which would be a lot cleaner than blocking thread
• Nkosi: You could also consider changing the approach and using a timer as apposed to a busy wait....just thinking aloud here

Answer 1

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?