Running async tasks and cancelling after a timeout if necessary - follow up (1)

Question

The previous question was a really bad design where I had all the logic in the Main method. The only way to test it was to look at the output in the console. I now encapsulated it in an extension method and wrote two unit-tests for it - here I'm not sure about the part using the Stopwatch for checking how long the execution took - do you think this is the right way to do it? I guess the implementation of the extension alone should now be better.

The purpose of this extension is to execute multiple jobs in parallel and cancel the ones that take too much time to complete.

public static void WaitAll(this IEnumerable<Action<CancellationToken>> actions, TimeSpan timeout)
{
 using (var cancellationTokenSource = new CancellationTokenSource())
 {
 var tasks = actions.Select(action => Task.Run(() => action(cancellationTokenSource.Token))).ToArray();
 if (!Task.WaitAll(tasks, timeout))
 {
 cancellationTokenSource.Cancel();
 // Wait for cancellation, if necessary.
 Task.WaitAll(tasks); 
 }
 }
}

I also have another overload that does not use the timeout.

public static void WaitAll(this IEnumerable<Action<CancellationToken>> jobs) => WaitAll(jobs, TimeSpan.FromMilliseconds(-1));

And the test class:

public class ActionExtensionsTest
{
 public TestContext TestContext { get; set; }
 [TestMethod]
 public void WaitAll_JobsFinishInTime_NoCancellation()
 {
 var jobs = new TestJob[]
 {
 new TestJob { WorkInSeconds = 6, Log = m => TestContext.WriteLine(m) },
 new TestJob { WorkInSeconds = 2, Log = m => TestContext.WriteLine(m) },
 new TestJob { WorkInSeconds = 4, Log = m => TestContext.WriteLine(m) },
 };
 var sw = Stopwatch.StartNew();
 jobs.Select(j => new Func<CancellationToken, Task>(j.Start)).WaitAll();
 Assert.IsTrue(sw.Elapsed.TotalSeconds < 7);
 }
 [TestMethod]
 [ExpectedException(typeof(AggregateException))]
 public void WaitAll_OneJobTimesout_Canceled()
 {
 var jobs = new TestJob[]
 {
 new TestJob { WorkInSeconds = 6, Log = m => TestContext.WriteLine(m) },
 new TestJob { WorkInSeconds = 2, Log = m => TestContext.WriteLine(m) },
 new TestJob { WorkInSeconds = 4, Log = m => TestContext.WriteLine(m) },
 };
 var sw = Stopwatch.StartNew();
 jobs.Select(j => new Func<CancellationToken, Task>(j.Start)).WaitAll(TimeSpan.FromSeconds(5));
 Assert.IsTrue(sw.Elapsed.TotalSeconds < 6);
 }
 class TestJob
 {
 private static int counter;
 static TestJob() => counter++;
 public string Name => $"{nameof(TestJob)}{counter}";
 public int WorkInSeconds { get; set; }
 public Action<string> Log { get; set; }
 public async Task Start(CancellationToken cancellationToken)
 {
 Log(Name + " started.");
 for (int i = 0; i < WorkInSeconds; i++)
 {
 cancellationToken.ThrowIfCancellationRequested();
 await Task.Delay(TimeSpan.FromSeconds(1));
 }
 Log(Name + " finished.");
 }
 }
}

• t3chb0t: Two downvotes already? Nice.

Answer 1

Looks good. One potential improvement is to make WaitAll method async. Maybe:

//return false on timeout
public static async Task<bool> RunAsync(this IEnumerable<Action<CancellationToken>> actions, TimeSpan timeout)

This way you can support two additional scenarios:

actions.RunAsync().Wait(); //sync wait (what you currently have)
await actions.RunAsync(); //await
actions.RunAsync().ContinueWith(...); //async call, no waiting

You will probably have to use Task.WhenAll instead of Task.WaitAll to make this happen.

Also 1s timeout is not something I would expect when I see actions.WaitAll() in code. Default timeout in all BCL classes I can think of is Infinite, you should probably follow this practice for consistancy.

• t3chb0t: Do you mean the -1 ms timeout? This means infinite according to the docs.
• Nikita B: @t3chb0t, yes. You can also use Timeout.Infinite constant.
• t3chb0t: Oh, sure ;-) Why do I see such things in someone else's code but not in my own? haha
• Nikita B: @t3chb0t occupational hazard ;)