InvokeAsync does not finally check CancellationToken

[HenKun]

In contrast to the other overloads, InvokeAsync in https://github.com/dotnet/winforms/blob/0ce1218949e478b130091479b7551eb1329804cb/src/System.Windows.Forms/src/System/Windows/Forms/Control_InvokeAsync.cs#L244 does not check the cancellationToken before finally invoking the delegate.

This should be done in order to

• be consistent
• avoid an unnecessary invocation

[kirsan31]

Here was some comments about InvokeAsync implementation (including cancellationToken). Didn't checked it by myself.

[HenKun]

It is even worse. It is not only an unnecessary invocation, it is completely unexpected.

InvokeAsync has already returned a cancelled task to the caller (through registering to the cancellation token).

Later on, the wrapped function passed to BeginInvoke is executed and with that the passed callback. This is unexpected to the caller and might result in confusing or wrong results on the GUI. It depends on the passed callback if (and how well) the passed cancellation token is handled.

Checking the token before invoking the callback prevents that in the first place.

But even introducing the check for the cancelled cancellation token in the wrapped function (as it is already in all overloads) still contains a slight race condition. If the token is cancelled after the check in the wrapped function, InvokeAsync returns a cancelled task, but still the callback gets executed.

[HenKun]

Here was some comments about InvokeAsync implementation (including cancellationToken). Didn't checked it by myself.

The issues seem similar but are no duplicates. They are orthogonal.

[kpreisser]

Hi,

InvokeAsync has already returned a cancelled task to the caller (through registering to the cancellation token).

Later on, the wrapped function passed to BeginInvoke is executed and with that the passed callback. This is unexpected to the caller and might result in confusing or wrong results on the GUI. It depends on the passed callback if (and how well) the passed cancellation token is handled.

But even introducing the check for the cancelled cancellation token in the wrapped function (as it is already in all overloads) still contains a slight race condition. If the token is cancelled after the check in the wrapped function, InvokeAsync returns a cancelled task, but still the callback gets executed.

Note that this behavior (the race condition) seems to be documented (at least that's how I interpret the documentation):

https://github.com/dotnet/winforms/blob/716bc129a4679a98394b2e2d24d1d1d3d4f54ddb/src/System.Windows.Forms/System/Windows/Forms/Control_InvokeAsync.cs#L75-L82

However, independent from this specific documentation I agree that this is rather an unexpected behavior: If the Task returned from InvokeAsync() is set to canceled, I would expect that the callback is not or no longer executed.

If it's not possible to actually stop the callback execution once it has started (which is the case here), then the Task probably shouldn't be set to Canceled, so that awaiting the task doesn't throw an exception while the callback is still executing, but instead the Task completes after the callback execution has finished. Hence, the only case where the Task could be set to Canceled would be when the token is cancelled before the callback actually started to execute, but not after it already started to execute.

[HenKun]

Exactly. This is how it should work.

But still, the behavior is currently different for the overloads in that all but one overload check the token again before executing the callback. This check makes the behavior reasonable for most of the cases but for the (more or less small) race condition.

[KlausLoeffelmann]

So, having read all this multiple times, I agree with y'all the behavior might not 100% optimal.

My question: Do you think we should tackle it to improve it (which I would absolutely be happy to do, if it made sense)? Please know, I championed the feature alright (just in case: Microsoft slang for guiding a feature to release, often also proposing it), but I am definitely listening (and also learning!), if there are things we can improve!

So, fire away!

[kpreisser]

Hi @KlausLoeffelmann,

thank you! Yes, I think we can improve the InvokeAsync() overloads to solve the mentioned issues.

Below is a possible revised InvokeAsync implementation (for the Func<T> callback variant as an example) which I believe should solve the mentioned issues (if I haven't missed anything):

public Task<T> InvokeAsync<T>(Func<T> callback, CancellationToken cancellationToken = default)
{
    ArgumentNullException.ThrowIfNull(callback);

    if (cancellationToken.IsCancellationRequested)
    {
        return Task.FromCanceled<T>(cancellationToken);
    }

    return InvokeAsyncCore();

    async Task<T> InvokeAsyncCore()
    {
        TaskCompletionSource<T> completion = new(TaskCreationOptions.RunContinuationsAsynchronously);
        int callbackInvoked = 0;

        using (cancellationToken.Register(CancellationCallback, useSynchronizationContext: false))
        {
            BeginInvoke(WrappedCallback);
            return await completion.Task.ConfigureAwait(false);
        }

        void CancellationCallback()
        {
            // Check that the callback invocation has not already started; otherwise,
            // do nothing since we can't stop the callback from being executed.
            // This ensures that once the callback has started to execute, the returned
            // `Task` won't complete until the callback has finished its execution.
            if (Interlocked.CompareExchange(ref callbackInvoked, 1, 0) != 0)
            {
                return;
            }

            completion.SetCanceled();
        }

        void WrappedCallback()
        {
            // Check that the cancellation callback has not already been executed;
            // otherwise, do nothing since this means the completion will be set to
            // Canceled by the cancellation callback.
            if (Interlocked.CompareExchange(ref callbackInvoked, 1, 0) != 0)
            {
                return;
            }

            try
            {
                T result = callback();
                completion.SetResult(result);
            }
            catch (Exception ex)
            {
                completion.SetException(ex);
            }
        }
    }
}

In particular, it should solve the following issues:

• By throwing the ArgumentNullException in a separate non-async method, this will actually be thrown as a regular exception, instead of returning a Task that has been set to an exception.
• If the cancellationToken has already been canceled when InvokeAsync is called, the method will return a canceled Task, instead of returning a default(T) value (as mentioned in https://github.com/dotnet/winforms/discussions/12496#discussion-7490694, point 1). AFAIK, this will result in a TaskCanceledException when awaiting the Task, which is also what other .NET methods do.
• By using Interlocked.CompareExchange() in both the cancellation callback and the wrapped callback, we ensure that the callback won't be executed when the Task has already been (or is going to be) canceled, and we ensure that the Task won't be canceled once the callback starts to execute (and will therefore complete only after execution of the callback has finished). This avoids the race condition/unexpected behavior mentioned in https://github.com/dotnet/winforms/issues/12696#issuecomment-2888798030.
  • Additionally, this synchronization means the TaskCompletionSource methods won't be called more than once, so we don't need to use the Try... methods to avoid the race condition that could result in an AggregateException being thrown by CancellationTokenSource.Cancel() (as mentioned in mentioned in https://github.com/dotnet/winforms/discussions/12496#discussion-7490694, point 2).

(Note however, that this does not yet solve the third issue mentioned in https://github.com/dotnet/winforms/discussions/12496#discussion-7490694.)

What do you think?

Thanks!

[HenKun]

Looks pretty good. Seems to solve all issues. To the third (unsolved) point: I think the IAsyncResult returned from BeginInvoke suffers from the same issue. EndInvoke would hang forever, if the thread terminates before the delegate is executed. If this was the case, then at least the behaviors would be consistent.