12
\$\begingroup\$

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;
    }
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Not really sure but shouldn't the SetException code be outside the foreach loop? \$\endgroup\$ Jul 21, 2014 at 11:48
  • \$\begingroup\$ Well your code almost looks like the decompiled one from Microsoft, did you have a look at it? =) \$\endgroup\$
    – DerApe
    Jul 21, 2014 at 13:08
  • \$\begingroup\$ I did indeed :) Was hoping it would be simpler, hence this attempt. \$\endgroup\$ Jul 21, 2014 at 18:14
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Jul 21, 2014 at 20:48
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 21, 2014 at 20:53

3 Answers 3

14
\$\begingroup\$
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();
\$\endgroup\$
2
  • \$\begingroup\$ Right. That makes it less confusing :) \$\endgroup\$ Jul 21, 2014 at 15:22
  • \$\begingroup\$ IsFaulted seems to indicate strictly that the task completed due to an exception (msdn.microsoft.com/en-us/library/…). \$\endgroup\$
    – usr
    Jul 22, 2014 at 9:06
11
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ And SetException() is called only once, outside the (inner) foreach loop. Was the question edited in the grace period? \$\endgroup\$
    – svick
    Jul 21, 2014 at 15:14
  • \$\begingroup\$ @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. \$\endgroup\$ Jul 21, 2014 at 15:18
  • \$\begingroup\$ Your update won't work correctly: exceptions is filled only after the last Task completes, but the outer foreach completes long before that. \$\endgroup\$
    – svick
    Jul 21, 2014 at 15:19
  • \$\begingroup\$ Ugh, true. Is there not a way to move it out of the loop though? \$\endgroup\$ Jul 21, 2014 at 15:21
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – svick
    Jul 21, 2014 at 15:22
4
\$\begingroup\$

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

if (Interlocked.Decrement(ref count) > 0)
{
    return;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – svick
    Jul 21, 2014 at 16:23
  • \$\begingroup\$ @svick you're right. Removed the crap. \$\endgroup\$ Jul 21, 2014 at 16:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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