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 am trying to write a version of Task.WhenAll for .NET 3.5 using the "Task Parallel Library for .NET 3.5". This is what I came up with. Is there a better way of doing this?

    public static Task WhenAll(IEnumerable<Task> tasks)
    {
        var tcs = new TaskCompletionSource<object>();

        var remainingTasks = tasks.ToList();
        int count = remainingTasks.Count();
        var exceptions = new List<Exception>();

        foreach (var task in remainingTasks)
        {
            task.ContinueWith(t =>
            {
                if (Interlocked.Decrement(ref count) == 0)
                {
                    foreach (var task1 in remainingTasks)
                    {
                        if (task1.IsFaulted)
                        {
                            exceptions.Add(task1.Exception);
                        }
                    }

                    if (exceptions.Any())
                    {
                        tcs.SetException(new AggregateException(exceptions));
                    }
                    else
                    {
                        tcs.SetResult(null);
                    }
                }
            });
        }

        return tcs.Task;
    }
share|improve this question
2  
Not really sure but shouldn't the SetException code be outside the foreach loop? –  craftworkgames Jul 21 at 11:48
    
Well your code almost looks like the decompiled one from Microsoft, did you have a look at it? =) –  derape Jul 21 at 13:08
    
I did indeed :) Was hoping it would be simpler, hence this attempt. –  Ned Stoyanov Jul 21 at 18:14
2  
Trying to outsmart library code is generally a losing game. Most of the time when you think you've found a simpler solution, what you've really done is removed the ability to handle one or more edge case failure. Unless you understand what the removed code is doing and are certain the case will never occur you're setting a time bomb in your code that will eventually go off. Generally at the worst possible moment. –  Dan Neely Jul 21 at 20:48
    
The library code wasn't available for .net 3.5. I had a look at the 4.0 version and emulated that. My version is of course inferior but hopefully returns the same result. –  Ned Stoyanov Jul 21 at 20:53

3 Answers 3

up vote 14 down vote accepted
int count = remainingTasks.Count();

Since remainingTasks is a List, you can use the Count property here.


var exceptions = new List<Exception>();

This variable should be declared where it's used: in the if Interlocked.Decrement block.


if (task1.IsFaulted)

This means you're treating canceled tasks the same as successfully completed ones. Instead, you can check whether Exception is null.

You could also rewrite the inner foreach using LINQ:

var exceptions = remainingTasks
    .Select(task => task.Exception)
    .Where(e => e != null)
    .ToList();
share|improve this answer
    
Right. That makes it less confusing :) –  Mat's Mug Jul 21 at 15:22
    
IsFaulted seems to indicate strictly that the task completed due to an exception (msdn.microsoft.com/en-us/library/…). –  usr Jul 22 at 9:06

I like how you've written this code. Just a couple of nitpicks:

Consistency with var

    var remainingTasks = tasks.ToList();
    int count = remainingTasks.Count();
    var exceptions = new List<Exception>();

Why not use var to declare count? Seems pretty obvious to me that remainingTasks.Count() would be an int, and it's the only explicitly typed variable in your method.

Naming

I don't like task1; you should prefer a meaninfgul name here, such as remainingTask. Also I'm not sure about tcs, I might have called it completionSource.

share|improve this answer
    
And SetException() is called only once, outside the (inner) foreach loop. Was the question edited in the grace period? –  svick Jul 21 at 15:14
    
@svick it's called inside the loop, every time a new exception is added to exceptions. Unless my reading is wrong. Updated answer with the SetException call outside the loop body. –  Mat's Mug Jul 21 at 15:18
    
Your update won't work correctly: exceptions is filled only after the last Task completes, but the outer foreach completes long before that. –  svick Jul 21 at 15:19
    
Ugh, true. Is there not a way to move it out of the loop though? –  Mat's Mug Jul 21 at 15:21
1  
It will be executed only once (after the last Task completes) in the original code; that's what the if (Interlocked.Decrement(ref count) == 0) does. –  svick Jul 21 at 15:22

I'm a fan of the fail-fast methodology, so I'd flip around the conditional to:

if (Interlocked.Decrement(ref count) > 0)
{
    return;
}
share|improve this answer
1  
1. The "real" WhenAll() is not an extension method, so this just mirrors that (and not making it an extension method makes sense to me). 2. I don't see how does using generics like that make it more flexible. A generic overload for Task<T> is useful, but that's not what you're proposing. –  svick Jul 21 at 16:23
    
@svick you're right. Removed the crap. –  Jesse C. Slicer Jul 21 at 16:41

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.