Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce InvokeAsync, ShowAsync, ShowDialogAsync #4631

Closed
Tracked by #10739
KlausLoeffelmann opened this issue Mar 3, 2021 · 51 comments
Closed
Tracked by #10739

Introduce InvokeAsync, ShowAsync, ShowDialogAsync #4631

KlausLoeffelmann opened this issue Mar 3, 2021 · 51 comments
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented
Milestone

Comments

@KlausLoeffelmann
Copy link
Member

KlausLoeffelmann commented Mar 3, 2021

Rationale

[Scenario and API proposal updated to reflect the current eco system.]

A growing number of components in and for WinForms requires to asynchronously marshal an async method to run on the UI-Thread. These are for example APIs around WebView2, projected native Windows 10 and 11 APIs or async APIs from modern libraries for example for Semantic Kernel.

Other scenarios make it necessary to show a Form, a Popup or a MessageBox asynchronously, to be in alignment of other UI stacks like WinUI or .NET MAUI and share/adapt their ViewModel implementations for migration and modernization purposes.

One example where we need to use these APIs is when we need to implement a Login-Dialog in WinForms to authenticate a user on a WebAPI which represents the WinForms App's backend, and which uses Windows WebView2 control and MSALs ICustomWebUI interface for acquiring an authorization code asynchronously.

Image

Since a modern architecture would it make necessary to call this Dialog via an UI-Service from a ViewModel, we would need to show this Form either with ShowAsync or ShowDialogAsync. Inside of the Form, when we would need to navigate to a specific URL and to honor the implementation of ICustomWebUI, we would need to call NavigateAsyncTo which would need to run a) asynchronously and b) on WinForms's UI Thread:

public partial class FormWebLogin : Form, ICustomWebUi
{
    public FormWebLogin()
    {
        InitializeComponent();
        InitializeBrowser();
    }

    /// <summary>
    ///  Implements the <see cref="ICustomWebUi.AcquireAuthorizationCodeAsync(Uri, Uri, CancellationToken)"/> method.
    /// </summary>
    public async Task<Uri> AcquireAuthorizationCodeAsync(
        Uri authorizationUri,
        Uri redirectUri,
        CancellationToken cancellationToken)
    {
        _redirectUri = redirectUri;

        // NavigateToAsync must be called asynchronously AND need to run on the UI thread.
        return await InvokeAsync<Uri>(
            () => NavigateToAsync(authorizationUri),
            cancellationToken);
    }

The following is a demo which utilizes InvokeAsync with the Periodic Timer, demos parallelized rendering into the WinForms's GDI+ Graphics surface and also shows off the new Windows title bar customizing (which is part of the Dark Mode features):

Image

API Suggestion:

Note: We suggest for .NET 9 to implement the new APIs under the Experimental attribute, so we can collect and react to respective feedback, and then introduce the new APIs finally in .NET 10.

namespace System.Windows.Forms;

public static partial class TaskDialog
{
    public static Task<TaskDialogButton> ShowDialogAsync(nint hwndOwner, TaskDialogPage page, TaskDialogStartupLocation startupLocation, CancellationToken cancellationToken);
    public static Task<TaskDialogButton> ShowDialogAsync(IWin32Window win32Window, TaskDialogPage page, TaskDialogStartupLocation startupLocation, CancellationToken cancellationToken);
    public static Task<TaskDialogButton> ShowDialogAsync(TaskDialogPage page);
    public static Task<TaskDialogButton> ShowDialogAsync(TaskDialogPage page, CancellationToken cancellationToken);
    public static Task<TaskDialogButton> ShowDialogAsync(TaskDialogPage page, TaskDialogStartupLocation startupLocation);
    public static Task<TaskDialogButton> ShowDialogAsync(TaskDialogPage page, TaskDialogStartupLocation startupLocation, CancellationToken cancellationToken);
}

public partial class Form
{
    public Task ShowAsync(IWin32Window? owner = null, CancellationToken cancellationToken = default);
    public Task<DialogResult> ShowDialogAsync();
    public Task<DialogResult> ShowDialogAsync(IWin32Window owner);
    public Task<DialogResult> ShowDialogAsync(CancellationToken cancellationToken);
    public Task<DialogResult> ShowDialogAsync(IWin32Window owner, CancellationToken cancellationToken);
}

public partial class Control
{
    // Awaiting the execution of the UI-Thread-marshalled `Action` or `Func`. 
    // If `Func` returns `Task[<TResult>]` it is also awaited.
    public Task InvokeAsync(Action action);
    public Task InvokeAsync(Action action, CancellationToken cancellationToken);
    public Task InvokeAsync(Func<Task> asyncFunc, CancellationToken cancellationToken);
    public Task<T> InvokeAsync<T>(Func<Task<T>> asyncFunc, CancellationToken cancellationToken);
    public Task<TResult> InvokeAsync<TResult>(Func<TResult> syncFunction);
    public Task<TResult> InvokeAsync<TResult>(Func<TResult> syncFunction, CancellationToken cancellationToken);
}
@KlausLoeffelmann KlausLoeffelmann added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Mar 3, 2021
@KlausLoeffelmann KlausLoeffelmann self-assigned this Mar 3, 2021
@dotnet dotnet locked and limited conversation to collaborators Mar 3, 2021
@KlausLoeffelmann KlausLoeffelmann changed the title [WIP] Introduce InvokeAsync on Control Introduce InvokeAsync on Control Mar 3, 2021
@dotnet dotnet unlocked this conversation Mar 3, 2021
@KlausLoeffelmann KlausLoeffelmann added the api-ready-for-review (2) API is ready for formal API review; applied by the issue owner label Mar 3, 2021
@KlausLoeffelmann

This comment has been minimized.

@RussKie RussKie added this to the 6.0 milestone Mar 3, 2021
@weltkante
Copy link
Contributor

weltkante commented Mar 3, 2021

public Task InvokeAsync(
    Func<Task> invokeDelegate,
    TimeSpan timeOutSpan = default,
    CancellationToken cancellationToken = default,
    params object[] args) 

The args make no sense, Func<Task> takes no arguments and if you pass anything but an empty array you'll get an exception (same for the other overload).

Also do you really want BeginInvoke behavior which always interrupts control flow (posting to the message loop) even if you are already on the UI thread? I mean its ok to do if you document it, people can do their own extension method which checks InvokeRequired just like they are doing currently. I only want to make sure its discussed which variant is expected to be the more common use case, immediate execution when already being on the UI thread, or always post like BeginInvoke.

If you divert from BeginInvoke behavior and execute the callback inline on the UI thread then you should also discuss returning ValueTask instead of class Task. I've not got any experience with ValueTask patterns so I'm just throwing this into the discussion for consideration not because its necessarily a good idea. (If you have BeginInvoke behavior and always post there is no point of ValueTask though).


Also (without seeing how the private InvokeAsync is called) I suspect this is an overly complex and probably bad approach to implement InvokeAsync, but shouldn't derail the API discussion, implementation can be discussed separately if desired. (For example I'm missing unrolling and awaiting of the Task returned by Func<Task> but maybe its done in the caller. Without having this the Task returned by InvokeAsync would complete before the async callback runs to completion.)

Cancellation also seems suspect

  • do we really need both timeout and cancellation token? there exist timeout cancellation tokens. I suspect the API is driven by the implementation here, and the implementation may be particularly bad.
  • the implementation of faking a cancellation/timeout after the callback started running seems questionable, I'm not aware of anything else in the framework doing this. As far as I'm aware cancellation is always done before the callback starts, once it starts the callback itself is responsible for observing cancellation. Faking early cancellation while the task still runs to completion in the background may cause issues.

Thats not to say cancellation isn't useful to have in this API, if you do BeginInvoke behavior of posting you may want to cancel your posted callback before it runs.

@terrajobst
Copy link
Member

terrajobst commented Mar 4, 2021

namespace System.Windows.Forms
{
    public partial class Control
    {
        public Task InvokeAsync(
            Func<Task> invokeDelegate,
            TimeSpan timeOutSpan = default,
            CancellationToken cancellationToken = default,
            params object[] args
        );
        public async Task<T> InvokeAsync<T>(
            Func<Task<T>> invokeDelegate,
            TimeSpan timeOutSpan = default,
            CancellationToken cancellationToken = default,
            params object[] args
        );
    }
}

@stephentoub
Copy link
Member

Just skimming the post now...

Am I understanding correctly that the timeout and CancellationToken don't actually impact the execution of the invokeDelegate, rather they cause the returned task to transition to a completed state even when the invokeDelegate may still be executing?

If so, that's exactly what the new WaitAsync methods will do:
dotnet/runtime#48842
e.g. if there's an existing Task-returning InvokeAsync method, you wouldn't need these overloads, and could instead just do:

await InvokeAsync(() => ...).WaitAsync(cancellationToken, timeout);

That WaitAsync takes the task from the InvokeAsync and returns a task that also incorporates cancellation / timeout, e.g. if cancellation is requested, the task returned from WaitAsync will be canceled even though the task returned from InvokeAsync may still be running.

@noseratio
Copy link

noseratio commented Mar 4, 2021

Oh, and this is the implementation I did so far with which I used in the demo:

Alternatively, I've been using the following version, modified to support timeOutSpan. I my experience, I never needed timeOutSpan, as CancellationToken was always good enough. Removing timeOutSpan could make it a bit shorter and perhaps a bit more performant, as the linked CancellationTokenSource would be gone too.

public static async Task<T?> InvokeAsync<T>(
    this Control @this,
    Delegate invokeDelegate,
    TimeSpan timeOutSpan = default,
    CancellationToken  = default,
    params object?[]? args)
{
    using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
    if (timeOutSpan != default)
    {
        cts.CancelAfter(timeOutSpan);
    }

    var tcs = new TaskCompletionSource<T?>(TaskCreationOptions.RunContinuationsAsynchronously);
    @this.BeginInvoke(new Action(() =>
    {
        try
        {
            // don't invoke the delegate if cancelled
            cts.Token.ThrowIfCancellationRequested();
            tcs.TrySetResult((T?)invokeDelegate.DynamicInvoke(args));
        }
        catch(Exception ex)
        {
            tcs.TrySetException(ex);
        }
    }), null);

    using (cts.Token.Register(() => tcs.TrySetCanceled()))
    {
        return await tcs.Task;
    }
}

Edited: addressing the comments, here is an updated version that supports both regular and async delegates for invokeDelegate.

@weltkante
Copy link
Contributor

weltkante commented Mar 4, 2021

Alternatively, I've been using the following version

Contrary to the original method (where I couldn't see the caller) this is public, thus definitely broken, because it doesn't unwrap and await the task returned by Func<Task> or Func<Task<T>> but instead returns a Task<Task>

When passing an actual async function the Task returned by InvokeAsync completes before the invokeDelegate completes.

In other words, your InvokeAsync does not support async callbacks, it supports calling synchronous callbacks in an async way. While in isolation it may be valid its different from how the rest of the dotnet framework developed.

I believe you shouldn't be writing a catch-all method handling arbitrary delegates. While you could try inspecting the return type of the delegate and "guess" based on its type whether you have to await it, that only works for Task and falls short for other cases. ValueTask and custom implementations will again cause the same problem of completing before the callback completes.

The IMHO correct solution is to only accept Func<Task> and Func<Task<T>>, by forcing the delegate signature to return Task (instead of accepting arbitrary delegates) you ensure the user gets a compiler error instead of runtime bugs. He has then a simple way to fix this compile error, by marking his method async and doing an await if necessary. This will always convert his method into the proper Task-returning signature.

@noseratio
Copy link

noseratio commented Mar 4, 2021

Contrary to the original method (where I couldn't see the caller) this is public, thus definitely broken, because it doesn't unwrap and await the task returned by Func<Task> or Func<Task<T>> but instead returns a Task<Task>

I guess I see what you mean. That version was for invoking synchronous delegates, and the real code accepts an Action (edited: Func<T?>), not an untyped Delegate:

static async Task<T?> InvokeAsync<T>(this Control @this, Func<T?> func, CancellationToken cancellationToken)

As for async lambdas, I use a different override (similar to how Task.Run does it):

public static async Task<T?> InvokeAsync<T>(
    this Control @this,
    Func<CancellationToken, Task<T?>> asyncFunc,
    CancellationToken cancellationToken = default)
{
    var tcs = new TaskCompletionSource<T?>(TaskCreationOptions.RunContinuationsAsynchronously);
    @this.BeginInvoke(new Action(async () =>
    {
        // we use async void on purpose here
        try
        {
            cancellationToken.ThrowIfCancellationRequested();
            tcs.TrySetResult((T?)await asyncFunc(cancellationToken));
        }
        catch (Exception ex)
        {
            tcs.TrySetException(ex);
        }
    }), null);

    using (cancellationToken.Register(() => tcs.TrySetCanceled()))
    {
        return await tcs.Task;
    }
}

The IMHO correct solution is to only accept Func and Func<Task>, by forcing the delegate signature to return Task (instead of accepting arbitrary delegates) you ensure the user gets a compiler error instead of runtime bugs. He has then a simple way to fix this compile error, by marking his method async and doing an await if necessary. This will always convert his method into the proper Task-returning signature.

Totally agree and that's how I do it in my projects, following the pattern of many others existing APIs in .NET.

Edited, as a matter of fact, @KlausLoeffelmann's version throws for the following code due to the same reason:

private async void Form1_Load(object sender, EventArgs e)
{
    // we're on UI thread
    await Task.Run(async () =>
    {
        // we're on thread pool
        var res = await this.InvokeAsync<bool>(new Func<Task<bool>>(async () => 
        {
            // we're on UI thread
            await Task.Delay(5000);
            return true;
        }));

        // we're on thread pool again
        await Task.Delay(1000);
    });

    // back on UI thread
    MessageBox.Show("Done");
}

BeginInvoke is unaware of the fact that the delegate is async and returns to the caller before Task.Delay(5000) is completed. This of course is solved with a proper override like I mentioned in this post.

@noseratio
Copy link

noseratio commented Mar 4, 2021

Contrary to the original method (where I couldn't see the caller) this is public, thus definitely broken, because it doesn't unwrap and await the task returned by Func<Task> or Func<Task<T>> but instead returns a Task<Task>

When passing an actual async function the Task returned by InvokeAsync completes before the invokeDelegate completes.

In other words, your InvokeAsync does not support async callbacks, it supports calling synchronous callbacks in an async way. While in isolation it may be valid its different from how the rest of the dotnet framework developed.

The original method seems to be broken too for async lambdas, as I've shown above. However, it's still possible to make it work for async lambdas disguised as untyped Delegates. It's a questionable design, but it's also the legacy of BeginInvoke.

Here's a take at that, it works for both regular and async delegates:

public static async Task<T?> InvokeAsync<T>(
    this Control @this,
    Delegate invokeDelegate,
    TimeSpan timeOutSpan = default,
    CancellationToken cancellationToken = default,
    params object?[]? args)
{
    using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
    if (timeOutSpan != default)
    {
        cts.CancelAfter(timeOutSpan);
    }

    var tcs = new TaskCompletionSource<T?>(TaskCreationOptions.RunContinuationsAsynchronously);

    async void Invoker() 
    {
        // async void makes sense here
        try
        {
            cts.Token.ThrowIfCancellationRequested();

            var result = invokeDelegate.DynamicInvoke(args);
            // if the result is a Task, await it
            if (result is Task task)
            {
                await task;
                tcs.TrySetResult(((Task<T?>)task).GetAwaiter().GetResult());
            }
            else
            {
                tcs.TrySetResult((T?)result);
            }
        }
        catch (Exception ex)
        {
            tcs.TrySetException(ex);
        }
    }

    @this.BeginInvoke(new Action(Invoker));

    using (cts.Token.Register(() => tcs.TrySetCanceled()))
    {
        return await tcs.Task;
    }
}

@KlausLoeffelmann
Copy link
Member Author

@weltkante

Also do you really want BeginInvoke behavior which always interrupts control flow (posting to the message loop) even if you are already on the UI thread?

Absolutely. One of the features I use Invoke for, and which I find super important in scenarios, where I want dedicatedly a method call to be getting event characteristics through scheduling it on the message queue. Even if not fired off from a different thread. I’ll add a rational for that as well in the nearer future, and I am really interested in a discussion about alternative approaches for those scenarios.

@weltkante
Copy link
Contributor

weltkante commented Mar 4, 2021

I am really interested in a discussion about alternative approaches for those scenarios.

await Task.Yield() will achieve the same thing and I personally prefer having it separate from InvokeAsync, but I'm not against keeping it in line with BeginInvoke and having users check InvokeRequired manually. Either way is fine, you always can implement the other semantic as extension method if you prefer it. I just want some discussion since from my perspective it feels like I want the inlined case more often, so getting other peoples perspective may be good.

// simplified pseudocode for how you would implement one if you have the other

// if InvokeAsync behaves as BeginInvoke as proposed you can check InvokeRequired
public static async Task InvokeAsync_Inline(Func<Task> callback)
{
  if (InvokeRequired)
    await InvokeAsync_Yield(callback);
  else
    await callback();
}

// the currently proposed semantic could be reimplemented as extension method if InvokeAsync would inline
public static async Task InvokeAsync_Yield(Func<Task> callback)
{
  if (!InvokeRequired)
  {
    await Task.Yield();
    await callback(); // since InvokeAsync_Inline(callback) would inline it anyways
  }
  else
  {
    await InvokeAsync_Inline(callback);
  }
}

If InvokeAsync does inline when already on the UI thread I would not expect people to write above extension method though, I'd expect people to explicitly call await Task.Yield(); if they want to interrupt the flow of execution. Its a bit like a less evil Application.DoEvents() (less evil because it forces the method to be async, making it clear to the caller that there is reentrancy)

@RussKie RussKie added design-discussion Ongoing discussion about design without consensus and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Mar 4, 2021
@RussKie
Copy link
Member

RussKie commented Mar 4, 2021

I wonder if something like @kevingosse proposed in his article would allow us achieve the desired in a different way?

@weltkante
Copy link
Contributor

weltkante commented Mar 4, 2021

The linked article is more like vs-threading using the pattern await SwitchToMainThread() and await SwitchToThreadPool() (names changed for clarity)

This pattern means you explicitly "change" threads by awaiting a custom awaiter, which will either be a no-op if you are on the right thread, or suspend execution and resume it on the right thread (by posting to the SynchronizationContext, which is equivalent to a BeginInvoke).

Having explicit async-await thread switches can be nice for readability in some scenarios, but make it worse in others since there is a hidden implication of the "current thread type" you have to keep in your mind when reading the source. We've been using the vs-threading library ever since it was open sourced (works for both WinForms and WPF and also solves deadlock issues you'd have with naive implementation of such thread switches), but I believe this approach is orthogonal to a one-off InvokeAsync and both approaches have reasons to exist.

@noseratio
Copy link

I wonder if something like @kevingosse proposed in his article would allow us achieve the desired in a different way?

That's a very interesting approach, thanks for the link. I personally like @kevingosse's initial idea, without the added AsyncMethodBuilder machinery:

private async Task UpdateControls()
{
    await Dispatcher.SwitchToUi();

    TextTime.Text = DateTime.Now.ToLongTimeString();
}

This kind of tells the intention right away, in a very readable way, IMHO. A similar API has been already proposed for .NET 6.0.

A potential problem I see with this is that we need to somehow remember the UI thread's task scheduler (TaskScheduler.FromCurrentSynchronizationContext). If we are already on a non-UI thread, and all we have is a Control object, the best thing we could then is to still use Control.Invoke to get it.

That said, a well-designed UI app should be updating the UI indirectly via a proper pattern (MVP/MVVM/MVU). So, we'd have a view model object, which could keep a references to the UI thread's task scheduler. That's where something like await uiTaskScheduler.SwitchTo() might be very useful.

@weltkante
Copy link
Contributor

weltkante commented Mar 4, 2021

A potential problem I see with this is that we need to somehow remember the UI thread's task scheduler

Only a problem if you want to build it as an external library, WinForms internally can have all the magic it wants, since it is providing the SynchronizationContext in the first place. That said BeginInvoke is a very cheap way to switch threads as long as you don't use the IAsyncResult (which is one reason why the implementation in the OP is bad). - In fact the SynchronizationContext and TaskScheduler use BeginInvoke for their implementation, so its the lowest primitive you can currently get.

However note that await control.SwitchToThreadAsync() (or whatever you want to name it) will have massively different semantics and programming styles than await control.InvokeAsync(... something to do ...):

await control.SwitchToThreadAsync()

  • is a custom awaitable/awaiter, not a Task you can store
  • lets you program in a "flow" programming style, not using delegates at all
  • requires you to maintain a strong mental model of the current thread you are on (which can be difficult once your code spans multiple methods, but also make things readable when everything is one method - can be both good and bad depending on how you use it)
  • you can already have something very close to this today (just not a method on Control) by referencing Microsoft.VisualStudio.Threading nuget package. don't mind the "Visual Studio" in the name, its just that this is where it came from, its available to everyone now and is open source

control.InvokeAsync(async () => { ... })

  • schedules an operation, awaiting it is optional
    • basically a more modern variant of BeginInvoke that can deal with async callbacks being passed in
    • you can await immediately, or launch multiple such operations and await later, or never await
    • allows branching flow instead of forcing linear flow

Like said above, I believe both approaches are valid, have their own advantages and can live alongside each other.

@RussKie
Copy link
Member

RussKie commented Mar 5, 2021

  • you can already have something very close to this today (just not a method on Control) by referencing Microsoft.VisualStudio.Threading nuget package. don't mind the "Visual Studio" in the name, its just that this is where it came from, its available to everyone now and is open source

Git Extensions has been using Microsoft.VisualStudio.Threading as well (gitextensions/gitextensions#4601, thanks to @sharwell), but it required additional tweaks and plumbing (e.g. ControlThreadingExtensions and ThreadHelper), and then further more tweaks to work reliably in UI tests. Those aren't the most straight forward implementations, and likely a lot of (Windows Forms) developers will find those intimidating.

@sharwell
Copy link
Member

sharwell commented Mar 5, 2021

My main concern with the InvokeAsync proposal is the same concern I still have with the Git Extensions approach: it's not obvious how these methods should tie in with closing/disposing forms, and I'm not convinced they will make it any easier for developers to address those concerns.

I will say: once the plumbing for vs-threading is in place, the code using that approach is dramatically cleaner than code trying to use InvokeAsync-style approaches.

@noseratio
Copy link

noseratio commented Mar 5, 2021

it's not obvious how these methods should tie in with closing/disposing forms, and I'm not convinced they will make it any easier for developers to address those concerns.

I think, the same concern applies to any async method which does anything on the UI thread.

I usually tackle this with the "main" cancellation token source which is triggered when the form is closing/disposing. With InvokeAsync, I imagine it could look like this:

async Task WorkAsync(CancellationToken outerToken, SomeForm form) {
    using var cts = CancellationTokenSource.CreateLinkedTokenSource(outerToken, form.MainToken);
    // ...
    await form.InvokeAsync(UpdateUiAsync, cts.Token);
    // ...
}

async Task UpdateUiAsync(CancellationToken token) {
    // e.g., we may need a delay for some debounce/throttle logic
    await Task.Delay(100, token); // this should throw if the form is gone 
    UpdateSomeUI();
    token.ThrowIfCancellationRequested(); // this should throw if the form is gone now
    UpdateSomeOtherUI();
}

InvokeAsync should be passing a cancellation token to its callback, whether the callback is sync or async. That's what I proposed in this code snippet.

I believe it's a good practice to make cancellation as "all way down" propagating as async/await itself should be.

@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented Mar 6, 2021

So, after a short email-exchange with @Pilchie, I tried the following based on a question he asked and I am wondering: what about this approach?

        public async Task InvokeAsync(
            Func<Task> invokeDelegate)
        {
            var asyncResult = BeginInvoke(invokeDelegate, args);
            _ = await Task<object>.Factory.FromAsync(asyncResult, EndInvoke);
        }

And, apart from the fact that discoverability of this as an alternative is obviously a problem, I am wondering: is it an alternative? What problems do you see here? If using this is OK, would we still need InvokeAsync?

@noseratio
Copy link

So, after a short email-exchange with @Pilchie, I tried the following based on a question he asked and I am wondering: what about this approach?

@KlausLoeffelmann I don't think this is going to work well, for the same reason I brought up here.

BeginInvoke is unware of async callbacks. As far as BeginInvoke is concerned, the call is done when a Func<Task> delegate returns a Task, not when the Task it returns is completed. To illustrate the problem, using the version of InvokeAsync you proposed above:

await Task.Run(async () =>
{
    // we're on thread pool
    await control.InvokeAsync(new Func<Task<bool>>(async () => {
        // we're on UI thread
        await Task.Delay(1000);
        Console.WriteLine("Updating");
        this.Text = "Updated";
        throw new Exception("Oh no");
    }));
});
Console.WriteLine("Done");

The output will be:

Updating
Done
Updated
<an exception is thrown but goes unobserved and lost>

I'd expect:

Updating
Updated
<an exception is thrown and propagated outside>

One other point. If I'm not mistaken, the Factory.FromAsync(asyncResult, EndInvoke) approach still makes use of IAsyncResult.AsyncWaitHandle, which leads to the creation of a ManualResentEvent event and all the overhead of asynchronously waiting for it to be signaled.

I keep pitching the TaskCompletionSource-based approach I proposed in various forms in this thread. It doesn't relay upon AsyncWaitHandle.

@weltkante
Copy link
Contributor

weltkante commented Mar 6, 2021

Just to be clear, for semantics I expect control.InvokeAsync(callback) to behave exactly as Task.Run(callback) - just on the controls UI thread instead of the thread pool. That means the returned Task is used for observing the callbacks completion and result, including exceptions. Inventing new semantics just used by WinForms will be confusing to users.

Furthermore, don't be lazy and try to offload the implementation of InvokeAsync onto already existing primitives, WinForms doesn't have code that supports async/await (or even understands Task) so there is nothing you can offload onto. You can and should use BeginInvoke to switch threads (because as explained above its the lowest level primitive WinForms has) - but do not use the IAsyncResult in any form, allocating that ManualResetEvent is extremely expensive (it needs interop and has a finalizer) and will be noticeable since its done for every call. The IAsyncResult is a leftover for compatibility and should not be used in modern code.

@UweKeim
Copy link

UweKeim commented May 13, 2023

Will public Task InvokeAsync(Func<Task> invokeDelegate) be available in WinForms for .NET 8?

@KlausLoeffelmann
Copy link
Member Author

It doesn't look too probable currently, to be honest.
We need to talk about this internally. @merriemcgaw, @JeremyKuhne FYI.

@paul1956
Copy link
Contributor

paul1956 commented Mar 7, 2024

This will also solve the issue with DownloadFile needing to call the new DownloadFileAsync and awaiting the results to be compatible with existing implementation.

@jnm2
Copy link
Contributor

jnm2 commented Mar 13, 2024

What do you think about an additional API pattern to cause the remainder of an async method to execute on the correct thread? It's very similar to how await Task.Yield() works, but instead of having a goal of yielding the thread and coming back, the goal would be to switch threads (only if not already on the correct thread).

Instead of writing:

// Maybe not on the UI thread now
await someControl.InvokeAsync(
    async () =>
    {
        // Definitely on the UI thread now
        await AbcAsync();
        await DefAsync();
    });
// Back to maybe not on the UI thread now

It would look like this:

// Maybe not on the UI thread now
await someControl.SwitchToThread();
// Definitely on the UI thread now
await AbcAsync();
await DefAsync();
// Still on the UI thread

Here's an implementation that is very similar in theory, around SynchronizationContext.Post rather than Control.BeginInvoke, but it could be quickly adapted: https://gist.github.com/jnm2/c0ea860c69af1230ac3c0b2d6d010b2a

There's a similar request for .NET itself for the "reverse" operation, moving to a background thread: dotnet/runtime#20025

Continuing to apply this pattern in another place, would you be open to adding await Application.WhenIdle() or similar? I can provide an example implementation for this as well. It would be a custom awaitable like the others, this time using the Application.Idle event to hook up the continuation for the rest of the method.

@KlausLoeffelmann
Copy link
Member Author

That sounds interesting. Give me some time to get my mind behind this!

@KlausLoeffelmann KlausLoeffelmann pinned this issue Jul 18, 2024
@KlausLoeffelmann KlausLoeffelmann changed the title Introduce InvokeAsync on Control Introduce InvokeAsync, ShowAsync, ShowDialogAsync Jul 18, 2024
@KlausLoeffelmann KlausLoeffelmann added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner blocking Used by the API Review Board labels Jul 18, 2024
@lonitra lonitra unpinned this issue Jul 23, 2024
@JeremyKuhne JeremyKuhne modified the milestones: .NET 9.0, 9.0 RC1 Jul 24, 2024
@KlausLoeffelmann KlausLoeffelmann pinned this issue Jul 30, 2024
@terrajobst
Copy link
Member

terrajobst commented Jul 30, 2024

Video

InvokeAsync

  • Should take a cancellation token in the callbacks
  • The callbacks should return value tasks
  • Cancellation token should be defaulted
  • Is not experimental
namespace System.Windows.Forms;

public partial class Control
{
    public Task InvokeAsync(Action callback, CancellationToken cancellationToken = default);
    public Task<T> InvokeAsync<T>(Func<TResult> callback, CancellationToken cancellationToken = default);
    public Task InvokeAsync(Func<CancellationToken, ValueTask> callback, CancellationToken cancellationToken = default);
    public Task<T> InvokeAsync<T>(Func<CancellationToken, ValueTask<T>> callback, CancellationToken cancellationToken = default);
}

ShowAsync/ShowDialogAsync

  • What happens if the token is cancelled?
    • The UI is being closed and the equivalent of DialogResult.Cancelled is returned.
    • That seems unfortunate and it's not what cancellation token are normally used for.
    • It seems better to leave off cancellation token for now.
  • TaskDialog
    • Aligned the overloads with the existing ShowDialog method, including defaults
  • What about MessageBox.ShowDialogAsync()?
    • Maybe letter; TaskDialog is meant to be the replacement for MessageBox.
  • What about CommonFileDialog.ShowDialogAsync()
    • That's missing and should probably be added.
  • Should be marked experimental
    • WFOXXXX is a placeholder; use a single diagnostic for all methods
namespace System.Windows.Forms;

public partial class TaskDialog
{
    [Experimental("WFOXXXX")]
    public static Task<TaskDialogButton> ShowDialogAsync(nint hwndOwner, TaskDialogPage page, TaskDialogStartupLocation startupLocation = TaskDialogStartupLocation.CenterOwner);

    [Experimental("WFOXXXX")]
    public static Task<TaskDialogButton> ShowDialogAsync(IWin32Window win32Window, TaskDialogPage page, TaskDialogStartupLocation startupLocation = TaskDialogStartupLocation.CenterOwner);

    [Experimental("WFOXXXX")]
    public static Task<TaskDialogButton> ShowDialogAsync(TaskDialogPage page, TaskDialogStartupLocation startupLocation = TaskDialogStartupLocation.CenterOwner);
}

public partial class Form
{
    [Experimental("WFOXXXX")]
    public Task ShowAsync(IWin32Window? owner = null);

    [Experimental("WFOXXXX")]
    public Task<DialogResult> ShowDialogAsync();

    [Experimental("WFOXXXX")]
    public Task<DialogResult> ShowDialogAsync(IWin32Window owner);
}

@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Jul 30, 2024
@KlausLoeffelmann KlausLoeffelmann unpinned this issue Aug 12, 2024
@JeremyKuhne JeremyKuhne removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation blocking Used by the API Review Board design-discussion Ongoing discussion about design without consensus labels Aug 12, 2024
@JeremyKuhne
Copy link
Member

@KlausLoeffelmann I believe #11854 resolves this.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented
Projects
None yet
Development

No branches or pull requests