3

I'd like to chain some tasks but conditionally continue with the execution if the CancellationToken I have wasn't fired.

What I'm aiming to achieve is something equivalent to

var cts = new CancellationTokenSource();
var cancellationToken = cts.Token;
var t = Task.Run(async () => {
    if (cancellationToken.IsCancellationRequested) return;
    await t1();
    if (cancellationToken.IsCancellationRequested) return;
    await t2();
    if (cancellationToken.IsCancellationRequested) return;
    await t3();
    if (cancellationToken.IsCancellationRequested) return;
    await t4();
});
var timeout = Task.Delay(TimeSpan.FromSeconds(4));
var completedTask = await Task.WhenAny(t, timeout);
if (completedTask != t)
{
    cts.Cancel();
    await t;
}

That's what I have by now and it is working, though it is also verbose.

0

6 Answers 6

1
var cts = new CancellationTokenSource();
var t = Task.Run(async () => {
     await t1();
     await t2();
     await t3();
     await t4();
 }, cts.Token);

cts.CancelAfter(TimeSpan.FromSeconds(4));

try
{
     await t;
}
catch (OperationCanceledException)
{
     // The cancellation token was triggered, add logic for that
     ....
}
2
  • 3
    Passing a CancellationToken as parameter of Task.Run has the effect of cancelling the Task only if it is still scheduled and hasn't started yet. So your code will either allow ALL tasks to run, or neither of them. This is not what the OP wants. Oct 19, 2019 at 10:28
  • 1
    Sorry pal, this does not do what I want. Oct 25, 2019 at 13:04
1

Your original code is correct-ish -- it assumes that you always want the individual tasks to run to completion and that if cancelled you want the overall task to complete with success. Neither of these things are idiomatic.

A more normal way to do it would be something like:

var cts = new CancellationTokenSource();
var cancellationToken = cts.Token;
var t = Task.Run(async () => {
    cancellationToken.ThrowIfCancellationRequested();
    await t1(cancellationToken);
    cancellationToken.ThrowIfCancellationRequested();
    await t2(cancellationToken);
    cancellationToken.ThrowIfCancellationRequested();
    await t3(cancellationToken);
    cancellationToken.ThrowIfCancellationRequested();
    await t4(cancellationToken);
}, cancellationToken);

Then, somewhere else:

cts.Cancel();

You can omit the calls to ThrowIfCancellationRequested here assuming that the individual tasks check it fairly soon after entry, but the core idea is that you should be passing the token down to the innermost loop of whatever is doing the work, and they should throw on cancellation by calling that, which ends up setting the task to a cancelled state rather than a success state.

(So really you only need to actually call ThrowIfCancellationRequested if you hit a function that doesn't accept a CancellationToken parameter -- which is why all async methods should do so, as otherwise its task will be non-cancellable.)

6
  • Your solution may propagate to the caller a surprising OperationCancelledException. This exception is suitable only if the caller is the initiator of the cancellation. This is not the case here. The cancellation happens as a result of the inner workings of the operation. The correct way to propagate the information that a timeout occurred, is by throwing a TimeoutException IMHO. I agree with passing the token into the individual async methods though. Aug 4, 2020 at 6:24
  • @TheodorZoulias Yes, if the time-based cancellation is kept then the whole thing should be wrapped and converted to a TimeoutException. From the context of the original question, however, a timeout is not the intended way this would actually be cancelled, it was just used for example purposes. I was merely rolling with that example.
    – Miral
    Aug 4, 2020 at 7:08
  • In the OP's example there is no exception thrown. In case of timeout the overall task completes with success. Which you claim to be non-idiomatic, and this is another point where I disagree with your answer (I believe it's idiomatic and bad practice). Aug 4, 2020 at 7:27
  • Which is why I'm saying that it should make the overall task complete with cancellation, which we both agree is better than success (which is the currently accepted answer). Again, I am assuming that the specific example of "cancel by timeout" was not the final intended method of cancellation. And as such, regardless of who cancelled the task, the task did get cancelled and this should be notified to anyone observing the task -- thus not surprising.
    – Miral
    Aug 4, 2020 at 7:32
  • I agree that cancellation is better than success, in the same sense that injury is better than death. Both are bad practices though. None communicates what really happened, which is: timeout. Aug 4, 2020 at 7:36
0

It seems like your goal is to just stop execution if the whole operation has taken longer than 4 seconds.

If you were passing the CancellationToken to your t1/t2/etc. methods, I'd say you couldn't do any better than what you have. But as you have it, you could just use a Stopwatch instead of a CancellationToken:

var timeout = TimeSpan.FromSeconds(4);
var stopwatch = new Stopwatch();
stopwatch.Start();

await t1();
if (stopwatch.Elapsed > timeout) return;
await t2();
if (stopwatch.Elapsed > timeout) return;
await t3();
if (stopwatch.Elapsed > timeout) return;
await t4();
stopwatch.Stop();

I assume this is in a method somewhere, where you can use return, but you can modify if needed (return a value, throw an exception, etc.).

3
  • Using a CancellationToken is a must for me in this case. Oct 25, 2019 at 13:04
  • @TiagoDall'Oca Why? Is this going to be called from other code that needs to pass a CancellationToken? Oct 25, 2019 at 13:12
  • Yes. This is some pattern that we might adopt while developing some functionalities that enable cancellation. Timeout here is really just a specific use case. Oct 25, 2019 at 13:23
0

Your code is functionally OK, but when reading it at a glance it's not clear what it's doing. So I suggest that you encapsulate this logic inside a utility method with descriptive name and parameters:

public static async Task RunSequentially(IEnumerable<Func<Task>> taskFactories,
    int timeout = Timeout.Infinite, bool onTimeoutAwaitIncompleteTask = false)
{
    using (var cts = new CancellationTokenSource(timeout))
    {
        if (onTimeoutAwaitIncompleteTask)
        {
            await Task.Run(async () =>
            {
                foreach (var taskFactory in taskFactories)
                {
                    if (cts.IsCancellationRequested) throw new TimeoutException();
                    await taskFactory();
                }
            });
        }
        else // On timeout return immediately
        {
            var allSequentially = Task.Run(async () =>
            {
                foreach (var taskFactory in taskFactories)
                {
                    cts.Token.ThrowIfCancellationRequested();
                    var task = taskFactory(); // Synchronous part of task
                    cts.Token.ThrowIfCancellationRequested();
                    await task; // Asynchronous part of task
                }
            }, cts.Token);
            var timeoutTask = new Task(() => {}, cts.Token);
            var completedTask = await Task.WhenAny(allSequentially, timeoutTask);
            if (completedTask.IsCanceled) throw new TimeoutException();
            await completedTask; // Propagate any exception
        }
    }
}

This code defers from yours in that it throws a TimeoutException on time-out. I think that it's better to force the caller to handle explicitly this exception, instead of hiding the fact that the operation timed-out. The caller can ignore the exception by leaving empty the catch block:

try
{
    await RunSequentially(new[] { t1, t2, t3, t4 },
        timeout: 4000,
        onTimeoutAwaitIncompleteTask: true);
}
catch (TimeoutException)
{
    // Do nothing
}
2
  • What you did here does seem to be a bit of an overkill but I liked the idea of putting tasks in a collection. Oct 25, 2019 at 13:06
  • @TiagoDall'Oca I didn't use a collection because each Task is awaited separately, and there is no reason to keep track of completed Tasks. I could have used a collection to hold the results of the Tasks though, if I had to do with Task<TResult>s, to return these results to the caller. In C# 8 not even this is necessary, since I could return an IAsyncEnumerable<TResult>, and sent the results to the caller one by one. Oct 25, 2019 at 16:29
0

You should consider using Microsoft's Reactive Framework (aka Rx) - NuGet System.Reactive and add using System.Reactive.Linq; - then you can do this:

IObservable<Unit> tasks =
    from x1 in Observable.FromAsync(() => t1())
    from x2 in Observable.FromAsync(() => t2())
    from x3 in Observable.FromAsync(() => t3())
    from x4 in Observable.FromAsync(() => t4())
    select Unit.Default;

IObservable<Unit> timer =
    Observable
        .Timer(TimeSpan.FromSeconds(4.0))
        .Select(x => Unit.Default)

IDisposable subscription =
    Observable
        .Amb(tasks, timer)
        .Subscribe();

If the timer observable fires before the tasks are complete then the entire pipeline is canceled. No unnecessary tasks are run.

If you want to cancel manually then just call subscription.Dispose().

The code is simple and beautiful too.

2
  • Yeah, it looks pretty good to me but I'm really looking for a way to use CancellationToken in my specific case. Very neat code though! Thx Oct 25, 2019 at 13:18
  • @TiagoDall'Oca - Why a CancellationToken? There's nothing about your tasks that you showed in your question that can be cancelled. In any case, it is possible to use Rx with a CancellationToken. I'd need to know more about what you're doing. Oct 25, 2019 at 23:30
-1

Since no one here gave a better answer I'll do it myself.

The code I provided in the question is, in my opinion, most cohesive and also general enough to be recycled in other cases.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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