1

I have the following method from an API which I have no control over.

public void Start(Action OnReady);

In general I am fine with callbacks but sometimes you have callback that triggers a callback and so on. Therefore I would like to wrap it into a async method and maybe also include the possibility to cancel the action. Something like this:

await Start(cancellationToken);

This is what I came up with:

public Task Start(CancellationToken cancellationToken)
{
     return Task.Run(() =>
     {
           cancellationToken.ThrowIfCancellationRequested();

           var readyWait = new AutoResetEvent(false);

           cancellationToken.Register(() => readyWait?.Set());

           Start(() => { readyWait.Set(); }); //this is the API method

           readyWait.WaitOne();
           readyWait.Dispose();
           readyWait = null;

           if(cancellationToken.IsCancellationRequested)
           {
                APIAbort(); //stop the API Start method from continuing
                cancellationToken.ThrowIfCancellationRequested();
            }

      }, cancellationToken);
}

I think there is room for improvement but one thing that comes to my mind is what does this method in this context?

readyWait.WaitOne(); 

I wanted to write an async method to not block any thread, but that's exactly what WaitOne does. Of course it does not block the calling thread because of the task, but does the task gets its own thread? I would be fine if only the task would be blocked, but I don't want to block a thread that might be in use somewhere else.

0lli.rocks
  • 1,027
  • 1
  • 18
  • 31
  • A callback already Async so why wrap a async method in a async method? Any event should not block. Blocking is done in main thread. An Event normally runs minimum amount of code and then returns. – jdweng Sep 11 '19 at 10:41
  • You could use TaskCompletionSource, https://stackoverflow.com/questions/15316613/when-should-taskcompletionsourcet-be-used – Viktor Arsanov Sep 11 '19 at 10:41

3 Answers3

3
public Task StartAsync(CancellationToken c)
{
    var cs = new TaskCompletionSource<bool>();

    c.Register(() => { Abort(); cs.SetCanceled(); } );
    Start(() => { cs.SetResult(true); });

    return cs.Task;
}
using (var ct = new CancellationTokenSource(1000))
{
    try
    {
        await StartAsync(ct.Token);

        MessageBox.Show("Completed");
    }
    catch (TaskCanceledException)
    {
        MessageBox.Show("Cancelled");
    }
}

There is no point to use the cancellation token because the only points where it can fire is immediately after the method is called and immediately before it's finished, at which point it's a race condition.

GSerg
  • 76,472
  • 17
  • 159
  • 346
  • I think this works around the WaitOne but how do I return early from this task if the Start method takes 10 minutes and I want to be able to interupt it? I think I could call SetResult in my Token.Register method, but could you explain the race condition? – 0lli.rocks Sep 11 '19 at 11:27
  • @0lli.rocks There is a way to support a cancellation token, but the problem is, you have called into an external routine `Start` that does not support tokens, and even if you cancel out of your `StartAsync` wrapper, the `Start` method is going to resume running and will eventually call the Action you passed, which is probably highly unwanted. – GSerg Sep 11 '19 at 11:37
  • That's why I am calling the Abort method which stops the Start method from continuing – 0lli.rocks Sep 11 '19 at 11:42
  • 1
    @0lli.rocks No, it does not. `Abort` requests to terminate the thread running inside `Task.Run`. This thread has already returned from `Start`. We don't know what happens inside `Start`, but apparently it spawns a worker thread of its own that will eventually call the Action; you do not terminate that worker thread. Besides, calling `Abort` is [never a good idea](https://stackoverflow.com/a/2253139/11683). – GSerg Sep 11 '19 at 11:51
  • 1
    I think the name of the method is confusing. it's not Thread.Abort it's another method from the API. I will clarify that in the question. Even though the name is similar the Abort does not stop any threads (even in the API) it will try to shut down the connection that Start started. – 0lli.rocks Sep 11 '19 at 11:53
  • 2
    @GSerg why not a single TCS? – Panagiotis Kanavos Sep 11 '19 at 12:04
  • @PanagiotisKanavos They call that get-there-itis in aviation; a failure to recognize that, upon a change in external conditions, your very goal should be reconsidered, rather than just the way to reach that goal. – GSerg Sep 11 '19 at 12:09
0

This problem is surprisingly tricky. The cases are many, and the handling of each case is not always obvious.

Obvious:

  1. The Start method may take a long time to complete. The same is true for the APIAbort method.
  2. The CancellationToken could be cancelled before, during, or after the Start method, or even after the OnReady callback invocation.
  3. The APIAbort should not be called before or during Start, or after the OnReady callback invocation.
  4. The APIAbort should be called even if the cancellation occured before completion of the Start method.

Not obvious:

  1. Should the returned Task complete as cancelled before or after the APIAbort invocation?
  2. What if APIAbort throws an exception? How to propagate this exception if the returned Task is already cancelled?
  3. What if Start throws an OperationCanceledException exception? Is this a fault or a cancellation?

The implementation below cancels the Task before invoking the APIAbort method, suppresses exceptions that may occur during APIAbort, and treats an OperationCanceledException during Start as cancellation.

public Task StartAsync(CancellationToken cancellationToken)
{
    if (cancellationToken.IsCancellationRequested)
        return Task.FromCanceled(cancellationToken);

    var tcs = new TaskCompletionSource<bool>();

    var cancellationRegistration = cancellationToken.Register(() =>
        tcs.TrySetCanceled(cancellationToken));

    var fireAndForget = Task.Run(() =>
    {
        if (cancellationToken.IsCancellationRequested) return;
        try
        {
            Start(() =>
            {
                cancellationRegistration.Dispose(); // Unregister
                tcs.TrySetResult(true);
            });
        }
        catch (OperationCanceledException)
        {
            tcs.TrySetCanceled();
            return;
        }
        catch (Exception ex)
        {
            tcs.TrySetException(ex);
            return;
        }

        // At this point Start is completed succesfully. Calling APIAbort is allowed.
        var continuation = tcs.Task.ContinueWith(_ =>
        {
            try
            {
                APIAbort();
            }
            catch { } // Suppressed
        },  default, TaskContinuationOptions.OnlyOnCanceled
        | TaskContinuationOptions.RunContinuationsAsynchronously,
        TaskScheduler.Default);
    }, cancellationToken);

    return tcs.Task;
}

The reason for setting the option TaskContinuationOptions.RunContinuationsAsynchronously is for avoiding the possibility of running the APIAbort synchronously (in the same thread) with the code after await StartAsync(). I run to this problem initially when I used async-await as a continuation mechanism.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
0

I would like to wrap it into a async method and maybe also include the possibility to cancel the action.

I recommend keeping those separate. The reason is that your "cancel" doesn't actually cancel the Start. It only cancels the wait for the Start to complete. So having a cancel at this level would be misleading.

You can wrap a delegate callback into a Task using a similar approach to the pattern for wrapping an event into a Task:

public static Task StartAsync(this ApiObject self)
{
  var tcs = new TaskCompletionSource<object>();
  self.Start(() => tcs.SetResult(null));
  return tcs.Task;
}

Now that you have a way to call StartAsync and get back a Task, you can choose not to continue waiting if you want:

var startTask = apiObject.StartAsync();
var timeoutTask = Task.Delay(TimeSpan.FromSeconds(10));
var completedTask = await Task.WhenAny(startTask, timeoutTask);
if (completedTask == timeoutTask)
  return;
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810