3

I'm working on a large monolith web application running on ASP.NET framework and added a feature. I've build some abstraction in order to run several tasks in parallel.

public interface IImporter
{
    Action[] GetProcessActions();
}

public class ImportTaskFactory : IDisposable
{
    private readonly Task[] _tasks;

    /// <summary>
    /// Immediately starts all the Process tasks
    /// </summary>
    public ImportTaskFactory(CancellationToken cancellationToken, Action<Task> onTaskFaultedAction, IEnumerable<IImporter> importers)
    {
        _tasks = importers
            .SelectMany(importer =>
                importer
                    .GetProcessActions()
                    .Select(action =>
                        Task.Factory.StartNew(action, cancellationToken)
                                    .ContinueWith(onTaskFaultedAction, TaskContinuationOptions.OnlyOnFaulted)))
            .ToArray();
    }

    public void WaitAll(CancellationToken cancellationToken)
    {
        Task.WaitAll(_tasks, cancellationToken);
    }

    public void Dispose()
    {
        foreach (var task in _tasks)
        {
            task?.Dispose();
        }
    }
}

and it's called like

//...
var importers = new IImporter[]
{
   //...three classes deriving from IImporter
}
using (var importTasks =
    new ImportTaskFactory(_cancellationTokenSource.Token, OnTaskFaulted, importers))
    {
        importTasks.WaitAll(_cancellationTokenSource.Token);
    }
//... where:
    private void OnTaskFaulted(Task task)
    {
        Log.Logline(task.Exception.ToString());
        _cancellationTokenSource.Cancel();
    }
//...

However, when running the tasks start out fine and do their work, but seem to stop for unclear reason, each throwing an System.Threading.Tasks.TaskCanceledException (no inner exception). If I look at the passed CancellationToken, cancellation was not requested.

enter image description here

I've been trying to debug this for a couple of hours, but have no idea what's happening. How can the tasks get cancelled this way? How can I debug this? I mean how can I see what triggers the cancellation?

I've set breakpoints in the OnTaskFaulted method, but it never seems to be called... That's the only point where _cancellationTokenSource.Cancel(); is called.

JHBonarius
  • 10,824
  • 3
  • 22
  • 41
  • 4
    What happens when you [remove](https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/) your `Dispose` and `using`? What further happens when you convert [the `StartNew` part](https://blog.stephencleary.com/2013/08/startnew-is-dangerous.html) into a boring code block with `await` and a `catch`? – GSerg May 07 '22 at 07:49
  • @GSerg You got it! it's the `StartNew`. Weird! I replaced it with `.Select(action =>{ var task = new Task(action, cancellationToken); task.ContinueWith(onTaskFaultedAction, TaskContinuationOptions.OnlyOnFaulted); task.Start(); return task;}))`. You want to write the answer so I can give you the creds? (note: our lead engineer want's no `await`s in our codebase...long story). – JHBonarius May 07 '22 at 08:04
  • 4
    *"our lead engineer want's no awaits in our codebase...long story"* -- That's a long story that I would be interested to hear! :-) – Theodor Zoulias May 07 '22 at 08:14
  • 1
    `new Task` is [not good either](https://stackoverflow.com/a/34146538/11683). `StartNew` ran all your tasks on individual threads, if you actually need that behaviour then convert your code to `Task.Run`, otherwise, do you even need a Task? The synchronous action that you are wrapping in a `Task` is [not going to respect](https://stackoverflow.com/a/30978029/11683) your cancellation token. – GSerg May 07 '22 at 08:17
  • 6
    @TheodorZoulias he doesn't understand them, doesn't seem to want to learn (and seems to be afraid of them). One of the many reasons I will only be working for this company for another 2 weeks :) – JHBonarius May 07 '22 at 08:17
  • 4
    Related: [Waiting on a Task with a OnlyOnFaulted Continuation causes an AggregateException](https://stackoverflow.com/questions/6573720/waiting-on-a-task-with-a-onlyonfaulted-continuation-causes-an-aggregateexception) – Theodor Zoulias May 07 '22 at 08:17
  • 1
    As a side note be aware that the `Task.WaitAll(_tasks, cancellationToken);` is not going to cancel the tasks. Is only going to cancel the *awaiting* of the tasks. The tasks themselves will keep going. – Theodor Zoulias May 07 '22 at 08:20
  • @TheodorZoulias yes, thanks. I know that one. That's why I do `task.ContinueWith` in the solution I show in the comments. Also: I'll look into the `WaitAll` that's probably a mistake. The tasks/actions themselves have cancellation implemented. To much to show here. – JHBonarius May 07 '22 at 08:20
  • 5
    I am confused. Writing *correct* code with continuations is much more difficult than with awaits, how come the lead engineer has mastered the former but not the latter?.. – GSerg May 07 '22 at 08:21
  • 2
    @GSerg He was automatically promoted to the role because every good developer left the company. He's the type that's often complaining he would like to go back to VB6, because he could understand that. I often submit some of his code to DailtWTF. I've talked to the boss about it, but it's not going to change, so that was my final cue to leave. – JHBonarius May 07 '22 at 08:26

1 Answers1

4

This not an answer to your question. It just shows how to do what you want, without getting these pesky cancellation exceptions:

void ImportTaskFactory(CancellationToken cancellationToken,
    Action<Task> onTaskFaultedAction, IEnumerable<IImporter> importers)
{
    _tasks = importers
        .SelectMany(importer => importer
            .GetProcessActions()
            .Select(action => Task.Run(() =>
            {
                try
                {
                    action();
                }
                catch (Exception ex)
                {
                    onTaskFaultedAction(Task.FromException(ex));
                }
            }, cancellationToken))).ToArray();
}

I replaced the Task.Factory.StartNew with Task.Run, because the former requires to pass explicitly the scheduler every time you use it.

Probably it would be simpler if the onTaskFaultedAction parameter was of type Action<Exception> instead of Action<Task>.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    Thanks. Can you for completeness also add what I should do if instead of actions, there would be a `importer.GetTasks` returning a `Task[]`? – JHBonarius May 07 '22 at 09:11
  • 2
    @JHBonarius if the `GetTasks` returns an array of `Task`, and you are not allowed to use async/await, you'll have to attach a `ContinueWith` continuation to each one of them, and take some action if `task.IsFaulted`. Life without async/await is sad. I am not even inclined to write down this code! – Theodor Zoulias May 07 '22 at 09:20
  • 1
    What would be the `async` way to wait on multiple tasks, but trigger a cancellationToken to cancel them all if any of them throws an exception? – JHBonarius May 07 '22 at 09:43
  • 2
    @JHBonarius check out this question: [How to properly cancel Task.WhenAll and throw the first exception?](https://stackoverflow.com/questions/42378265/how-to-properly-cancel-task-whenall-and-throw-the-first-exception) – Theodor Zoulias May 07 '22 at 09:51