2

I'm designing the software architecture for a product who can instantiate a series of "agents" doing some useful things. Let's say each agent implement an interface having a function:

Task AsyncRun(CancellationToken token)

Because since these agents are doing a lot of I/O it could make some sense having as an async function. More over, the AsyncRun is supposed never complete, if no exception or explict cancellation occour.

Now the question is: main program has to run this on multiple agents, I would like to know the correct way of running that multiple task, signal each single completion ( that are due to cancellation/errors ): for example I'm thinking on something like having an infinite loop like this

//.... all task cretaed are in the array tasks..
while(true)
{
     await Task.WhenAny(tasks)
     //.... check each single task for understand which one(s) exited
     // re-run the task if requested replacing in the array tasks
}

but not sure if it is the correct ( or even best way ) And moreover I would like to know if this is the correct pattern, especially because the implementer can mismatch the RunAsync and do a blocking call, in which case the entire application will hang.

Felice Pollano
  • 32,832
  • 9
  • 75
  • 115
  • _"the implementer can mismatch the RunAsync and do a blocking call"_ as he can with quite all Async APIs. Exactly that is why it is recommended **not** to do so. You can also stress that point in your docs/manual and refer to "RTFM" if people start barking up your tree. But there isn't much you can do to actively prevent the client from doing it. – Fildor Jun 24 '19 at 13:03
  • *"main program has to run this on multiple agents"*. Does this means that each agent will invoke the `AsyncRun` method independently from the other agents, or that all agents will make a single invocation somehow? – Theodor Zoulias Jun 24 '19 at 13:12
  • @Fildor is my strategy not recomemnted, or having a blocking async function? – Felice Pollano Jun 24 '19 at 13:39
  • 3
    Stephen C says: [don't-block-on-async-code](https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html). I don't want to commit an attribution-to-authority fallacy, but when it comes to async, I tend to trust him. – Fildor Jun 24 '19 at 13:44
  • @TheodorZoulias I think I've clarified a bit the question – Felice Pollano Jun 24 '19 at 13:57
  • This is a really interesting question and I'm watching to see some great answer. But the title is too vague to say anything about the question. I recommend making the title more specific. – Scott Hannen Jun 24 '19 at 14:28
  • Have you considered [observer pattern (ReactiveX)](http://reactivex.io/)? – Peter Wolf Jun 24 '19 at 14:47
  • @TheodorZoulias tasks, providing that no one is blocking, aren't running independently? – Felice Pollano Jun 24 '19 at 15:07
  • 1
    @FelicePollano I think that your pattern is fine, provided that you don't need the flexibility to stop and restart specific agents when arbitrary events occur (for example from user input). You can only control the tasks when one of them completes, and you don't know when this will happen next. – Theodor Zoulias Jun 24 '19 at 15:23
  • @TheodorZoulias well consider creating a reply, even more, RunAsync has a CancellationToken for the purpose you are saying – Felice Pollano Jun 24 '19 at 15:24
  • @FelicePollano I am not talking about abruptly cancelling running jobs, I am talking about stopping gracefully the recurrent flow of repeating runs of a particular agent, and restarting it later on demand. Is this a desirable feature for your application? I am gathering the requirements before attempting to offer an answer! – Theodor Zoulias Jun 24 '19 at 15:31

2 Answers2

0

In order to know whether the task completes successfully, or is cancelled or faulted, you could use a continuation. The continuation will be invoked as soon as the task finishes, whether that's because of failure, cancellation or completion. :

using (var tokenSource = new CancellationTokenSource())
{
    IEnumerable<IAgent> agents; // TODO: initialize

    var tasks = new List<Task>();
    foreach (var agent in agents)
    {
        var task = agent.RunAsync(tokenSource.Token)
            .ContinueWith(t =>
            {
                if (t.IsCanceled)
                {
                    // Do something if cancelled.
                }
                else if (t.IsFaulted)
                {
                    // Do something if faulted (with t.Exception)
                }
                else
                {
                    // Do something if the task has completed.
                }
            });

        tasks.Add(task);
    }

    await Task.WhenAll(tasks);
}

In the end you will wait for the continued tasks. Also see this answer.

If you are afraid that the IAgent implementations will create blocking calls and want to prevent the application from hanging, you can wrap the call to the async method in Task.Run. This way the call to the agent is executed on the threadpool and is therefore non-blocking:

var task = Task.Run(async () =>
    await agent.RunAsync(tokenSource.Token)
        .ContinueWith(t =>
        {
            // Same as above
        }));

You may want to use Task.Factory.StartNew instead to mark the task as longrunning for example.

Jesse de Wit
  • 3,867
  • 1
  • 20
  • 41
  • How will that last snippet prevent the client from `.Result` the Task returned? – Fildor Jun 24 '19 at 14:05
  • It will not. It will only make sure that a non-asynchronous agent does not make your application hang, because it uses the threadpool. There is no way to prevent someone from writing poor code, in your code. – Jesse de Wit Jun 24 '19 at 14:08
  • But it _will_ make client's code "hang" if he's just doing `.Result` on the Task returned. No difference if it's wrapped in Task.Run or not. So it doesn't solve the problem but deprives you of creating IO-Only async code without extra thread. (I did not downvote, btw) – Fildor Jun 24 '19 at 14:11
  • I read you were afraid that implementers of the interface would make blocking calls and your code calls the interface. In that case the snippet would work. – Jesse de Wit Jun 24 '19 at 14:15
  • @Fildor I believe you have misread the question. (S)he doesn't even mention a client – Jesse de Wit Jun 24 '19 at 14:16
  • Ah, I see. That's where you are coming from. Yes, I was more like talking about "calling the interface blockingly" rather than "use blocking code to be executed" if that makes sense. But nevertheless, I would rather have the client do some research than "just in case"-wrap everything in Task.Run. But that's just me. – Fildor Jun 24 '19 at 14:17
  • @Fildor what do you mean 'do some research'? If the agents are added on a plugin basis, there's no way to control the execution other than wrapping in a seperate task. – Jesse de Wit Jun 24 '19 at 14:30
  • "Research" is meant like client code has blocking call => will block. Client will (hopefully) see a performance issue and research into it. If they are clever, they will soon find out, they are not supposed to block ... The question is: Can we expect clients to be clever enough or will this throw off most of them? That's why I also recommended to state it explicitly in docs and manual. "Harsh love" as some coworker of mine would say. – Fildor Jun 24 '19 at 14:37
0

// re-run the task if requested replacing in the array tasks

This is the first thing I'd consider changing. It's far better to not let an application handle its own "restarting". If an operation failed, then there's no guarantee that an application can recover. This is true for any kind of operation in any language/runtime.

A better solution is to let another application restart this one. Allow the exception to propagate (logging it if possible), and allow it to terminate the application. Then have your "manager" process (literally a separate executable process) restart as necessary. This is the way all modern high-availability systems work, from the Win32 services manager, to ASP.NET, to the Kubernetes container manager, to the Azure Functions runtime.

Note that if you do want to take this route, it may make sense to split up the tasks to different processes, so they can be restarted independently. That way a restart in one won't cause a restart in others.

However, if you want to keep all your tasks in the same process, then the solution you have is fine. If you have a known number of tasks at the beginning of the process, and that number won't change (unless they fail), then you can simplify the code a bit by factoring out the restarting and using Task.WhenAll instead of Task.WhenAny:

async Task RunAsync(Func<CancellationToken, Task> work, CancellationToken token)
{
  while (true)
  {
    try { await work(token); }
    catch
    {
      // log...
    }

    if (we-should-not-restart)
      break;
  }
}

List<Func<CancellationToken, Task>> workToDo = ...;
var tasks = workToDo.Select(work => RunAsync(work, token));
await Task.WhenAll(tasks);
// Only gets here if they all complete/fail and were not restarted.

the implementer can mismatch the RunAsync and do a blocking call, in which case the entire application will hang.

The best way to prevent this is to wrap the call in Task.Run, so this:

await work(token);

becomes this:

await Task.Run(() => work(token));
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810