-1
\$\begingroup\$

I need to call some synchronization function and need they running in background and canbe canceled. So I write this:

private static async Task WaitSyncFunction(Action syncFunction,
    int timeoutMilliseconds,
    CancellationToken token)
{
    var syncFunctionTask = Task.Run(syncFunction);
    var timeoutTask = Task.Delay(timeoutMilliseconds, token);
    await Task.WhenAny(timeoutTask, syncFunctionTask).ContinueWith(task =>
    {
        if (timeoutTask.IsCanceled) throw new TaskCanceledException();
        if (timeoutTask.IsCompletedSuccessfully) throw new TimeoutException();
        if (syncFunctionTask.IsFaulted) throw syncFunctionTask.Exception.InnerException;
    });
}

use it:

//...async method of button click event handler in window
cancelSource = new CancellationTokenSource();
try
{
    await WaitSyncFunction(() => MySyncFunction(), 5000, cancelSource.Token);
    MessageBox.Show("Success!");
}
catch (TaskCanceledException) { MessageBox.Show("Canceled!"); }
catch (Exception ex) { MessageBox.Show("Error! "+ ex.Message); }
//...

What risks may exist in codes, and is it anti-pattern?

============Edit============
Some improvements:

//Custom exception for process task after timeout
public class TaskTimeoutException : TimeoutException
{
    public Task task { get; }
    public TaskTimeoutException(Task task)
        => this.task = task;
}

private static async Task WaitSyncFunction(Action syncFunction,
    int timeoutMilliseconds,
    CancellationToken token)
{
    var syncFunctionTask = Task.Run(syncFunction);
    var timeoutTask = Task.Delay(timeoutMilliseconds, token);
    await Task.WhenAny(timeoutTask, syncFunctionTask);//.ContinueWith(task =>
    //{ //Unnecessary ContinueWith
    //if (timeoutTask.IsCanceled) throw new TaskCanceledException();
    //return the function task so that it can do something else after it ending
    if (timeoutTask.IsCanceled) throw new TaskCanceledException(syncFunctionTask);
    //if (timeoutTask.IsCompletedSuccessfully) throw new TimeoutException();
    //return the function task so that it can do something else after it ending
    if (timeoutTask.IsCompletedSuccessfully) throw new TaskTimeoutException(syncFunctionTask);
    if (syncFunctionTask.IsFaulted) throw syncFunctionTask.Exception.InnerException;
    //});
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Have you read this article? \$\endgroup\$ Mar 4, 2021 at 12:39
  • 1
    \$\begingroup\$ Welcome to the Code Review Community. There really isn't enough code here to review. Please read How do I ask a good question? and What types of questions should I avoid asking. \$\endgroup\$
    – pacmaninbw
    Mar 4, 2021 at 12:51
  • \$\begingroup\$ @PeterCsala I've read it but I think it may not much fit for me, in my code it support 4 status: Canceled, Faulted, Timeout and Complated. but the code in article is only support 3 status: Canceled, Faulted and Complated. If i want to add timeout I need add a Task to achieve. I think it's not a good idea. (CancelAfter also not a good idea, it will be think as Canceled, not Timeout) \$\endgroup\$ Mar 5, 2021 at 2:13

1 Answer 1

3
\$\begingroup\$

Whenever we have an async method and we want to differentiate Canceled from Timeout then we usually do the following:

  1. We are anticipating OperationCanceledException (the base class of TaskCanceledException)
  2. We examine the IsCancellationRequested property of the CancellationToken

Let me show you a simple example:

Timeout

private static readonly TimeSpan OperationDuration = TimeSpan.FromSeconds(3);
// timeoutSource will be triggered
private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2); 
private static readonly TimeSpan CancelAfter = TimeSpan.FromSeconds(10);

static async Task Main(string[] args)
{
    var userCancellationSource = new CancellationTokenSource(CancelAfter);
    try
    {
        await TestAsync(userCancellationSource.Token);
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine(userCancellationSource.IsCancellationRequested ? "Canceled" : "Timed out");
    }
}

public static async Task TestAsync(CancellationToken token = default)
{
    var timeoutSource = new CancellationTokenSource(Timeout);
    var timeoutOrCancellationSource = CancellationTokenSource.CreateLinkedTokenSource(timeoutSource.Token, token);
    await Task.Delay(OperationDuration, timeoutOrCancellationSource.Token);
}

Canceled

private static readonly TimeSpan OperationDuration = TimeSpan.FromSeconds(3);
private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2); 
// userCancellationSource will be triggered 
private static readonly TimeSpan CancelAfter = TimeSpan.FromSeconds(1); 

static async Task Main(string[] args)
{
    var userCancellationSource = new CancellationTokenSource(CancelAfter);
    try
    {
        await TestAsync(userCancellationSource.Token);
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine(userCancellationSource.IsCancellationRequested ? "Canceled" : "Timed out");
    }
}

public static async Task TestAsync(CancellationToken token = default)
{
    var timeoutSource = new CancellationTokenSource(Timeout);
    var timeoutOrCancellationSource = CancellationTokenSource.CreateLinkedTokenSource(timeoutSource.Token, token);
    await Task.Delay(OperationDuration, timeoutOrCancellationSource.Token);
}

I think the same pattern should be followed by your wrapper. To have the same behavior I have modified your WaitSyncFunction method in the following way

private static async Task WaitSyncFunction(Action syncFunction, int timeoutMilliseconds, CancellationToken token)
{
    var syncFunctionTask = Task.Run(syncFunction);
    var timeoutTask = Task.Delay(timeoutMilliseconds, token);
    await Task.WhenAny(timeoutTask, syncFunctionTask);
    if (timeoutTask.IsCanceled) token.ThrowIfCancellationRequested(); //changed
    if (timeoutTask.IsCompletedSuccessfully) throw new OperationCanceledException(token); //changed
    if (syncFunctionTask.IsFaulted) throw syncFunctionTask.Exception.InnerException;
}

Timeout

private static readonly TimeSpan OperationDuration = TimeSpan.FromSeconds(3);
// timeoutSource will be triggered
private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2);
private static readonly TimeSpan CancelAfter = TimeSpan.FromSeconds(10);

static async Task Main(string[] args)
{
    var userCancellationSource = new CancellationTokenSource(CancelAfter);
    try
    {
        await WaitSyncFunction(() => Thread.Sleep((int) OperationDuration.TotalMilliseconds),
            (int) Timeout.TotalMilliseconds, userCancellationSource.Token);
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine(userCancellationSource.IsCancellationRequested ? "Canceled" : "Timed out");
    }
}

private static async Task WaitSyncFunction(Action syncFunction, int timeoutMilliseconds,
    CancellationToken token)
{
    var syncFunctionTask = Task.Run(syncFunction);
    var timeoutTask = Task.Delay(timeoutMilliseconds, token);
    await Task.WhenAny(timeoutTask, syncFunctionTask);
    if (timeoutTask.IsCanceled) token.ThrowIfCancellationRequested();
    if (timeoutTask.IsCompletedSuccessfully) throw new OperationCanceledException(token);
    if (syncFunctionTask.IsFaulted) throw syncFunctionTask.Exception.InnerException;
}

Canceled

private static readonly TimeSpan OperationDuration = TimeSpan.FromSeconds(3);
private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(2);
// userCancellationSource will be triggered 
private static readonly TimeSpan CancelAfter = TimeSpan.FromSeconds(1);

static async Task Main(string[] args)
{
    var userCancellationSource = new CancellationTokenSource(CancelAfter);
    try
    {
        await WaitSyncFunction(() => Thread.Sleep((int) OperationDuration.TotalMilliseconds),
            (int) Timeout.TotalMilliseconds, userCancellationSource.Token);
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine(userCancellationSource.IsCancellationRequested ? "Canceled" : "Timed out");
    }
}

private static async Task WaitSyncFunction(Action syncFunction, int timeoutMilliseconds,
    CancellationToken token)
{
    var syncFunctionTask = Task.Run(syncFunction);
    var timeoutTask = Task.Delay(timeoutMilliseconds, token);
    await Task.WhenAny(timeoutTask, syncFunctionTask);
    if (timeoutTask.IsCanceled) token.ThrowIfCancellationRequested();
    if (timeoutTask.IsCompletedSuccessfully) throw new OperationCanceledException(token);
    if (syncFunctionTask.IsFaulted) throw syncFunctionTask.Exception.InnerException;
}

Please bear in mind that with this implementation the syncFunction is not aborted in case of timeout. To support cooperative cancellation you have to pass the CancellationToken to the syncFunction and examine its IsCancellationRequested property periodically (at each checkpoint / milestone).

Check out these for more details:

\$\endgroup\$
2
  • \$\begingroup\$ If I want to do something process after the "MySyncFunction" end (complated or faulted), I should write a override... WaitSyncFunction(Task syncFunctionTask, ...) and create the task of sync function before calling? (Throw a exception with a task looks weird indeed) \$\endgroup\$ Mar 8, 2021 at 1:28
  • \$\begingroup\$ @Squirrel.Downy To be honest with you I'm not sure I've understood your question 100%, could you please rephrase it. If you want to execute something after completion then call it after your await WaitSyncFunction( ...); inside the try block. If you want to execute something in case of failure then call it inside the catch block. \$\endgroup\$ Mar 8, 2021 at 7:49

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