Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following method, that has been shorten for the sake of this review. The actual method has many more tasks. Essentially what I am trying to do is to create many tasks, fire them off, wait until they are all finished, and handle any exceptions generated. Does this look correct?

public static void RefreshCoaterCaches()
{
    try
    {
        CicApplication.CoaterDataLock.EnterWriteLock();

        List<Task> alarmTasks = new List<Task>();

        alarmTasks.Add(Task.Factory.StartNew(() =>
        {
            using (DistributorBackpressure44Logic logic = new DistributorBackpressure44Logic())
                logic.RefreshDistributorBackpressure44Cache();
        }));

        alarmTasks.Add(Task.Factory.StartNew(() =>
        {
            using (DeltaPressureLogic logic = new DeltaPressureLogic())
                logic.RefreshDeltaPressureCache();
        }));

        Task.WaitAll(alarmTasks.ToArray());
    }
    catch (System.AggregateException exception)
    {
        CicServerLogic.HandleUnexpectedAggregateException(exception.Flatten(), string.Empty);
    }
    catch (System.Exception exception)
    {
        CicServerLogic.HandleUnexpectedException(exception);
    }
    finally
    {
        CicApplication.CoaterDataLock.ExitWriteLock();
    }
}

}

I then have this method to handle any AggregateException thrown:

internal static void HandleUnexpectedAggregateException(AggregateException aggregateException)
{
    try
    {
        foreach (Exception exception in aggregateException.InnerExceptions) 
        {
         ...
         ...
         Do some stuff with the current exception, like logging etc.
         ...
        }
    }
    catch (Exception)
    {
    }
}
share|improve this question
    
I'm not 100%, but won't the first exception stop the unfinished tasks from running? –  dreza Nov 27 '12 at 1:52
    
@dreza: not it not. When first exception occur all started tasks will continue to execute. –  Sergey Teplyakov Nov 27 '12 at 11:18
add comment

1 Answer

I suggested to think about following:

  1. Extract every step into the separate asynchronous method
  2. Think about more careful locking technique. Asynchronous operations by its nature could be long running and using global write lock can degrade performance dramatically. Maybe its possible acquire locks only during processing results.
  3. Global exception handling: in general global exception handling is a code smell. Its really hard to create generic logic for different operations. I suggested to use such techniques with some tools like MEF, but avoid using its manually.

Here slightly modified version of your code:

private static Task GetDistributorBackpressure44LogicAsync()
{
  Task.Factory.StartNew( () =>
  {
    using (DistributorBackpressure44Logic logic = new DistributorBackpressure44Logic())
                logic.RefreshDistributorBackpressure44Cache();
  }
}

private static Task DeltaPressureLogicAsync()
{
  Task.Factory.StartNew(() =>
  {
    using (DeltaPressureLogic logic = new DeltaPressureLogic())
                logic.RefreshDeltaPressureCache();
  }
}

public static void RefreshCoaterCaches()
{
    try
    {
        CicApplication.CoaterDataLock.EnterWriteLock();

        var t1 = GetDistributorBackpressure44LogicAsync();
        var t2 = DeltaPressureLogicAsync();

        // You can switch back to you list-based approach if you'll have
        // much more other operations
        Task.WaitAll(t1, t2);
    }
    catch (System.AggregateException exception)
    {
        // I'm really not sure that its a good idea to create "generic"
        // exception handling scheme. Maybe you should handle them manually right here.
        // And what is string.Empty is all about?
        CicServerLogic.HandleUnexpectedAggregateException(exception.Flatten(), string.Empty);
    }
    // You should not catch System.Exception thats why I removed it
    finally
    {
        CicApplication.CoaterDataLock.ExitWriteLock();
    }
}

Now, about your generic exception handler (I mentioned already that this technique can lead to errors hiding and complicated "generic" logic) (see my comments right in the code):

internal static void HandleUnexpectedAggregateException(AggregateException aggregateException)
{
    // What are we afraiding of, why we're using try/catch here?
    // We should now swallow unknown exception because it can lead to
    // hiding bugs in our code!
    try
    {
        // This technique is ok
        foreach (Exception exception in aggregateException.InnerExceptions) 
        {
         ...
         ...
         Do some stuff with the current exception, like logging etc.
         ...
        }
    }
    catch (Exception)
    {
       // I strongly not recommended to use empty catch blocks!!
    }
}
share|improve this answer
    
Thank you. What's the advantage of creating separate asynchronous methods? –  Randy Minder Nov 27 '12 at 20:26
    
@RandyMinder: because its easier to read, compose (for example with new async features), reuse, easier to refactor to real async implementation (instead of calling synchronous methods asynchronously you can provide some other implementations later). –  Sergey Teplyakov Nov 27 '12 at 20:28
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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