Doubts about Control.InvokeAsync implementation

[KalleOlaviNiemitalo]

The .NET 9 release notes for Windows Forms mentioned that Control.InvokeAsync #4631 is now available. Out of curiosity, I looked at how it was implemented, but I have some doubts about whether it’s correct.

1. If given a CancellationToken whose cancellation has already been requested, Control.InvokeAsync appears to return a Task that has already run to completion. I’d expect it to return a canceled task, instead.
2. There seems to be a race condition in cancellation: WrappedCallback might notice the cancellation request first and call completion.TrySetCanceled, and then completion.SetCanceled might also be called because of CancellationToken.Register; but the completion was already set so SetCanceled would throw in this case. Perhaps this could be demonstrated by first registering another cancellation callback that sleeps a bit to give WrappedCallback some time to act before SetCanceled is called.
3. If InvokeAsync calls BeginInvoke, but the thread that owns the target Control then terminates before the window message is handled, I think the Task returned by InvokeAsync will hang and never complete (unless canceled via CancellationToken). I’d expect the Task to fail with InvalidAsynchronousStateException. This looks unfixable as long as InvokeAsync uses BeginInvoke, but could perhaps be fixed by saving the TaskCompletionSource into the ThreadMethodEntry and faulting it if the queue is cleared without callbacks being executed.

[elachlan]

@KlausLoeffelmann will be interested in the feedback on InvokeAsync

[KlausLoeffelmann]

Sorry I missed this.
Most useful comments to a feature I ever got - thanks so much, I really appreciate that.
@merriemcgaw: I'll take a look, do some tests, and will create an issue, if necessary (which I think is likely).

[kpreisser]

Hi!

I recently started using Control.InvokeAsync() in one of my apps, and also stumbled upon point 1, that when passing an already canceled CancellationToken (or one that is canceled soon after calling the method), the returned Task would complete with a null/default value instead of being set to the canceled state, so I agree that this should be fixed to return a canceled task instead (maybe by calling CancellationToken.ThrowIfCancellationRequested()?).

Regarding point 2: Note that the race condition is not only due to TrySetCanceled() call in WrappedCallback, but also due to the TrySetResult() call that happens after the callback was executed: If the token is canceled between the TrySetResult() call (after the callback execution is complete) and the point where the CancellationTokenRegistration is disposed, the SetResult call from the CancellationTokenRegistration will throw an InvalidOperationException, which can result in an AggregateException being thrown by CancellationTokenSource.Cancel().
(This can be reproduced by inserting a Thread.Sleep() call between awaiting the completion.Task and disposing the CancellationTokenRegistration.)

I think one possibility to fix the race condition is by using TrySetCanceled instead of SetCanceled in the CancellationTokenRegistration callback, so that if either TrySetCanceled or TrySetResult already happened in the WrappedCallback, this won't throw an InvalidOperationException.

Thanks!

[HenKun]

To extend this: On

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Control_InvokeAsync.cs

Line 53 in 0ce1218

completion.SetCanceled(cancellationToken);

also SetCanceled is used. The other overloads are already using TrySetCanceled at this location.

To the part to use CancellationToken.ThrowIfCancellationRequested(). We have to be careful when this is called. If this is called before the first await happens, the exception is raised in the calling context of InvokeAsync, not when the returned task is awaited by the user! The method runs synchronously until the first await. So this would result in a difference between:

await InvokeAsync(....)

and

var task = InvokeAsync(...); <- OperationCanceledException is thrown here!
await task; 

Also, I created an issue with similar and related issues, but not duplicates in #12696. Maybe that can somehow be merged in the discussion to find a solution that considers all issues.

[kpreisser]

Hi @HenKun ,

also SetCanceled is used. The other overloads are already using TrySetCanceled at this location.

👍

To the part to use CancellationToken.ThrowIfCancellationRequested(). We have to be careful when this is called. If this is called before the first await happens, the exception is raised in the calling context of InvokeAsync, not when the returned task is awaited by the user!

I'm not sure if I follow: As the InvokeAsync() methods are marked async, calling CancellationToken.ThrowIfCancellationRequested() in that method when the token is canceled won't result in directly throwing a OperationCanceledException by Control.InvokeAsync(), but instead results in returning a Task that is set to canceled (and awaiting the task would then throw the exception). In your example, this would mean:

var task = InvokeAsync(...); // Returns a canceled task without throwing
await task; // <- OperationCanceledException is thrown here

If the method wasnt't marked as async then you would be right, as then the exception would be thrown directly without returning a Task. In that case, we would have to use something like

if (cancellationToken.IsCancellationRequested)
    return Task.FromCanceled(cancellationToken);

However, an actual difference between calling CancellationToken.ThrowIfCancellationRequested() and returning a canceled task from a TaskCompletionSource where cts.[Try]SetCanceled() was called (or when using Task.FromCanceled(..) seems to be that the former results in an OperationCanceledException whereas the others result in a TaskCanceledException (which is a subclass of the former). However, I think I've also seen this difference/inconsistency in other async .NET APIs, though I don't recall on which. For a consistent behavior, I think the exception to be thrown should be TaskCanceledException.

[HenKun]

You are right. Marking the method async cares for returning a canceled task instead of throwing directly, even if the exception is raised before the first await. I was wrong.

[KalleOlaviNiemitalo]

I tried defining an async Task function that creates a CancellationTokenSource, cancels it, and asks its CancellationToken to throw OperationCanceledException. In the resulting Task, Task.Status is TaskStatus.Canceled, and Task.Exception is null. Awaiting the Task rethrows the OperationCanceledException, not a TaskCanceledException.

I'm not sure about the circumstances in which TaskCanceledException would be thrown. Perhaps it's only used if a CancellationToken is provided during task creation and the task is canceled via that before the task ever starts executing.

The Task Cancellation topic claims that the task instance "compares the exception's token to its associated token". In my test, the exception's CancellationToken was created by the async function itself, rather than provided in a parameter. Any such comparison would then have detected a mismatch. Despite that, the task entered TaskStatus.Canceled.

[HenKun]

@KalleOlaviNiemitalo yes your observation is right.

I am not sure if this is a problem at all. Clients need to handle OperationCanceledException either way. That Task.Run throws a TaskCanceledException on cancellation before the delegate is executed is an implementation detail for me.