1
\$\begingroup\$

I know embarassingly little about asynchronous programming in C#, so decided to start catching up. It would help me a lot if anyone checked this basic example I've created.

Assumption: we're creating a simulation where the time passes turn by turn with predefined minimal interval, e.g. 1 second. However, if any important calculations during the turn would take longer than this interval, next turn should not begin until they're done.

I would be grateful if you could tell me if there any visible issues, code smells, or if it can be optimized. I've read bad things about using Task.Run, but I don't really know how to tackle it better.

public static class Program
{
    static void Main()
    {
        var gameTurnsController = new GameTurnsController();
        var cts = new CancellationTokenSource();

        try
        {
            // Asynchronously wait for input to send cancellation token
            Task.Run(() =>
            {
                Console.ReadKey(true);
                cts.Cancel();
            });

            gameTurnsController.PassTurns(TimeSpan.FromSeconds(1), cts.Token).Wait(cts.Token);
        }
        // It smells of controlling the execution flow through exception. Isn't there a better way to handle cancellation?
        catch (OperationCanceledException)
        {
            Console.WriteLine(Environment.NewLine + "Cancelled.");
            Environment.ExitCode = 0;
        }
        
        Console.WriteLine("Execution finished.");
    }
}
public class GameTurnsController
{
    public async Task PassTurns(TimeSpan interval, CancellationToken cancellationToken)
    {
        while (true)
        {
            if (cancellationToken.IsCancellationRequested)
            {
                return;
            }

            // Wait until both delay and calculation is finished
            await Task.WhenAll(Task.Delay(interval, cancellationToken), LongRunningOperation(cancellationToken));
        }
    }

    // expensive method example
    private async Task<long> LongRunningOperation(CancellationToken cancellationToken)
    {
        return await Task.Run(() =>
        {
            long result;
            // ...
            // expensive math operation that returns long value
            // ...
            return result;
        }, cancellationToken);
    }
}

In the above code I've removed most of the logs and stopwatches for clarity.

\$\endgroup\$
2
  • \$\begingroup\$ What .NET and C# version are you using? I'm asking it because for example async main is supported since C# 7.1 \$\endgroup\$ Feb 18, 2021 at 9:28
  • \$\begingroup\$ I'm using 7.0, but I know about async main. Do I get it right that the only change here would be awaiting the PassTurns call instead of using .Wait on it? \$\endgroup\$
    – BFyre
    Feb 18, 2021 at 9:54

1 Answer 1

3
\$\begingroup\$

The Wait call is tricky.

  • It throws by default AggregateException not OperationCanceledException.
  • If you provide the cancellationToken to the Wait then it will throw an OperationCanceledException.
  • The biggest problem with this approach is that it cancels only the Wait not the underlying task.
    • Please read this article for more details.

So, if we switch to async main then the application will not stop after user cancellation:

static async Task Main()
{
    var gameTurnsController = new GameTurnsController();
    var userRequestedCancellation = new CancellationTokenSource();

    try
    {
        _ = Task.Run(() =>
        {
            Console.WriteLine("Please press any key to cancel");
            Console.ReadKey(true);
            userRequestedCancellation.Cancel();
        });

        await gameTurnsController.PassTurns(TimeSpan.FromSeconds(1), userRequestedCancellation.Token);
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine(Environment.NewLine + "Cancelled.");
        Environment.ExitCode = 0;
    }

    Console.WriteLine("Execution finished.");
}
  • Because the underlying LongRunningOperation will not terminate.
    • (Let me use Thread.Sleep(5000) to simulate complex CPU bound operation)
private async Task<long> LongRunningOperation(CancellationToken cancellationToken)
{
    return await Task.Run(() =>
    {
        long result;
        Thread.Sleep(5000);
        return result;
    }, cancellationToken);
}
  • If I run this app and I press any key then it will not cancel the LongRunningOperation method that's why the output will be the Execution finished..

How to fix it?

By passing all the way long the cancellationToken:

private async Task<long> LongRunningOperation(CancellationToken cancellationToken)
{
    var job = Task.Run(() => LongRunningOperationInternal(cancellationToken), cancellationToken);
    return await job;
}

private long LongRunningOperationInternal(CancellationToken cancellationToken)
{
    long result = 1;
    for (int i = 0; i < 5; i++)
    {
        cancellationToken.ThrowIfCancellationRequested();
        Thread.Sleep(1000); 
    }

    return result;
}

If need to do graceful cancellation then consider to use the following pattern:

if(cancellationToken.IsCancellationRequested)
{
  //TODO: clean up whatever is needed

  cancellationToken.ThrowIfCancellationRequested();
}

TL;DR: You need to check the cancellationToken at milestones/checkpoints to be able to truly cancel the whole application.


Might be an interesting read: How to can non-cancelable async operations

\$\endgroup\$
0

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.