Simplify Invoke and BeginInvoke signature and accept Action

[RussKie]

Proposed API:

Add an overload that takes Action as a parameter for both Invoke(Action method) and BeginInvoke(Action method).
I don't think it is necessary to create overloads for methods that take params object[] args as it is possible to pass necessary parameters via a closure.

+        public IAsyncResult BeginInvoke(Action method)
         public IAsyncResult BeginInvoke(Delegate method)
         public IAsyncResult BeginInvoke(Delegate method, params object[] args)

+        public T Invoke<T>(Func<T> method)
+        public void Invoke(Action method)
         public object Invoke(Delegate method)
         public object Invoke(Delegate method, params object[] args)

Background

There are two golden rules for Windows Forms:

1. Always interact with UI controls on the same thread as they are created. Interacting with controls from another thread requires InvokeRequired, Invoke, BeginInvoke, etc.
2. Never execute a long-running piece of code in the UI thread.

Invoke and BeginInvoke take Delegate as their input parameters, which requires rather cumbersome boilerplate code, and frankly look dated by todays standard.

E.g.

private void UpdateUI() 
{
    if (myControl.InvokeRequired) 
    {
        myControl.Invoke(new MethodInvoker(() => { UpdateUI(); })); // <-- this is very cumbersome
    } 
    else
    {
        // we're on UI thread, update myControl
    }
}

⚠️ NB: The discussion of automatically switching to the UI thread is well outside the scope of this proposal. It will come separately.

The cumbersome bit is this:

myControl.Invoke(new MethodInvoker(() => UpdateUI()));
float area = 0;
listView1.Invoke(new Action(() => { area = CalculateArea(); }));

which can also be written as

myControl.Invoke((MethodInvoker)(() => { UpdateUI(); }));
myControl.Invoke((Action)(() => { UpdateUI(); }));

However it can not be written as this:

myControl.Invoke(() => UpdateUI());
// or 
myControl.Invoke(UpdateUI); // slightly inefficient - the delegate is not cached, https://github.com/dotnet/roslyn/issues/5835

despite the fact Action is of a delegate type, just like MethodInvoker Thank you @weltkante for pointing it out.

float area = (float)listView1.Invoke(new Func<float>(() => CalculateArea()));

But now it will be possible to write something like this:

float area1 = listView1.Invoke(() => CalculateArea(width, length));
//
float area2 = listView1.Invoke(CalculateArea);

Perf considerations

The added benefit of the new API is that we will be creating smaller code footprint:

        private void InvokeDelegateFunc(object sender, EventArgs e)
        {
            // current
            float area = (float)listView1.Invoke(new Func<float>(() => CalculateArea()));
        }

        private void InvokeFunc(object sender, EventArgs e)
        {
            // proposed
            float area2 = listView1.Invoke(CalculateArea);
        }

InvokeDelegateFunc

.method private hidebysig 
	instance void InvokeDelegateFunc (
		object sender,
		class [System.Runtime]System.EventArgs e
	) cil managed 
{
	// Method begins at RVA 0x7104
	// Code size 31 (0x1f)
	.maxstack 3
	.locals init (
		[0] float32 area
	)

	// sequence point: (line 193, col 9) to (line 193, col 10) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0000: nop
	// sequence point: (line 194, col 13) to (line 194, col 102) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0001: ldarg.0
	IL_0002: ldfld class [System.Windows.Forms]System.Windows.Forms.ListView WinformsControlsTest.ListViewTest::listView1
	IL_0007: ldarg.0
	IL_0008: ldftn instance float32 WinformsControlsTest.ListViewTest::'<InvokeDelegateFunc>b__5_0'()
	IL_000e: newobj instance void class [System.Runtime]System.Func`1<float32>::.ctor(object, native int)
	IL_0013: callvirt instance object [System.Windows.Forms]System.Windows.Forms.Control::Invoke(class [System.Runtime]System.Delegate)
	IL_0018: unbox.any [System.Runtime]System.Single
	IL_001d: stloc.0
	// sequence point: (line 195, col 9) to (line 195, col 10) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_001e: ret
} // end of method ListViewTest::InvokeDelegateFunc

.method private hidebysig 
	instance float32 '<InvokeDelegateFunc>b__5_0' () cil managed 
{
	.custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
		01 00 00 00
	)
	// Method begins at RVA 0x7a7c
	// Code size 7 (0x7)
	.maxstack 8

	// sequence point: (line 194, col 83) to (line 194, col 98) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0000: ldarg.0
	IL_0001: call instance float32 WinformsControlsTest.ListViewTest::CalculateArea()
	IL_0006: ret
} // end of method ListViewTest::'<InvokeDelegateFunc>b__5_0'

vs

InvokeFunc

.method private hidebysig 
	instance void InvokeFunc (
		object sender,
		class [System.Runtime]System.EventArgs e
	) cil managed 
{
	// Method begins at RVA 0x7130
	// Code size 26 (0x1a)
	.maxstack 3
	.locals init (
		[0] float32 area2
	)

	// sequence point: (line 198, col 9) to (line 198, col 10) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0000: nop
	// sequence point: (line 199, col 13) to (line 199, col 59) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0001: ldarg.0
	IL_0002: ldfld class [System.Windows.Forms]System.Windows.Forms.ListView WinformsControlsTest.ListViewTest::listView1
	IL_0007: ldarg.0
	IL_0008: ldftn instance float32 WinformsControlsTest.ListViewTest::CalculateArea()
	IL_000e: newobj instance void class [System.Runtime]System.Func`1<float32>::.ctor(object, native int)
	IL_0013: callvirt instance !!0 [System.Windows.Forms]System.Windows.Forms.Control::Invoke<float32>(class [System.Runtime]System.Func`1<!!0>)
	IL_0018: stloc.0
	// sequence point: (line 200, col 9) to (line 200, col 10) in C:\Development\winforms\src\System.Windows.Forms\tests\IntegrationTests\WinformsControlsTest\ListViewTest.cs
	IL_0019: ret
} // end of method ListViewTest::InvokeFunc

Other considerations

I have checked emitted IL, and it appears to be of the same size, with the only difference use of Action instead of MethodInvoker.

@KlausLoeffelmann has highlighted to me that the issue appears to be C# specific, as VB understand the following right now:

        Friend Sub Test()
            Dim control As New Button()
            control.BeginInvoke(Sub()
                                    MessageBox.Show("Test!")
                                End Sub)
        End Sub

I have verified the proposal does not appear to have any negative impacts on VB.

[RussKie]

@MadsTorgersen @jaredpar @KathleenDollard can you see any issues with this change?

[jaredpar]

LGTM

[kirsan31]

#1347 😁

[tbolon]

myControl.Invoke(() => { UpdateUI(); });

Will this also works?

myControl.Invoke(UpdateUI);

[weltkante]

Will this also works?

Yes but its currently slightly more inefficient because the delegate is not cached (for historic reasons). There's a roslyn issue for changing this and making it behave like the other delegate syntax variants.

[KlausLoeffelmann]

@weltkante, do you think if we introduced this, we would break a terribly awful lot of people with regards to what you wrote here?

#1347 (comment)

(And let's all keep in mind, we always said, given the nature of .NET in contrast to Framework, we don't want to, but if the benefit outweighs the risk, we would introduce breaking changes. I'd say it would do here, so I am very much in favor to take this.)

[weltkante]

do you think if we introduced this, we would break a terribly awful lot of people with regards to what you wrote here?

Not really, no, people who wrote an extension method will compile against the new overload now (has higher priority than the extension method), but I really doubt someone would make an Invoke(Action) extension on Control and give it different behavior than Invoke(Delegate)

I expect existing extension methods are doing the same you are going to do in your overload so that should not be breaking.

Users who don't recompile will still be calling their extension method regardless of what you introduce here, so at most its affecting source code, and you can be expected to update your code when you compile against a new framework version. (Though I don't think its necessary in this case to update anything for most users.)

The comment in the other thread was just saying that having this method inbox is "nice to have" and people are already doing it manually via extension, but it wasn't an argument against introducing it.

[tbolon]

Will this also works?

Yes but its currently slightly more inefficient because the delegate is not cached (for historic reasons). There's a roslyn issue for changing this and making it behave like the other delegate syntax variants.

Sorry for the digression; but I completely forgot about this, and I remember how I was shocked, because I was always thinking that this form was more efficient that the other one (instead of allocating a delegate, just point directly to the method). Thanks for the reminder 😅

[RussKie]

Updated with public T Invoke<T>(Func<T> method) overload.

[weltkante]

I think you missed that the old Invoke method works with any delegate type, not just with MethodInvoker.

I have no idea how Invoke returning a result is written, it's so cumbersome ...

You'd either cast the boxed value or return it through a variable captured in the closure:

float area = 0;
listView1.Invoke(new Action(() => { area = CalculateArea(); }));
// consume "area"

or

var area = (float)listView1.Invoke(new Func<float>(() => CalculateArea()));

despite the fact Action is of a delegate type, just like MethodInvoker.

MethodInvoker has nothing to do with the original methods, they take any delegate. MethodInvoker is not special in any regards, you could replace it literally with Action:

myControl.Invoke(new Action(() => { UpdateUI(); }));

or

myControl.Invoke((Action)(() => { UpdateUI(); }));

in fact you can pass (almost) arbitrary delegates, taking parameters or returning values. Thats why there is an overload taking a params-array.

For what its worth its not necessary to add an Action<...> overload taking a params array, because it'd be less efficient than the original overload (as mentioned in my previous comment on the other issue). If you are looking for performance you preallocate the delegate without closure and pass arguments explicitly, but if you preallocate the delegate then you can use the existing overload.

[RussKie]

Thank you @weltkante, I indeed totally overlooked it.
I've update the proposal. I think there are some perf benefits to reap as well.

[terrajobst]

Video

• Looks good as proposed
  • Is this a source breaking change for VB? @KathleenDollard @jaredpar? Jared already said looks good. But it looks like @KathleenDollard still has come concerns. This should be verified.

namespace System.Windows.Forms
{
    public partial class Control
    {
        public IAsyncResult BeginInvoke(Action method);
        // public IAsyncResult BeginInvoke(Delegate method);
        // public IAsyncResult BeginInvoke(Delegate method, params object[] args);

        public T Invoke<T>(Func<T> method);
        public void Invoke(Action method);
        // public object Invoke(Delegate method);
        // public object Invoke(Delegate method, params object[] args);
    }
}

[RussKie]

@weltkante made a great comment in #4631 (comment)

...if you do BeginInvoke behavior of posting you may want to cancel your posted callback before it runs.

This could be addressed by dotnet/runtime#47525 that's looking at adding support for timeout and cancellation token support.

[weltkante]

This could be addressed by dotnet/runtime#47525 that's looking at adding support for timeout and cancellation token support.

Actually I don't think so, that will only allowing the consumer of the Task to specify when it wants to abort waiting on the Task (to stop waiting when some other condition is met), but the callback will still be running to completion - which for WinForms is especially bad because it will be running to completion on the UI thread. Being able to actually cancel the callback to ensure its never executed if it didn't start yet will not be possible with that API, in order to support that it must be implemented on your side.

WPF has had the ability from the start, their BeginInvoke returns a DispatcherOperation (instead of just an IAsyncResult) and you can call Abort on that.

I mean its not a big deal that WinForms doesn't have it, whenever I need to cancel a BeginInvoke I just define a boolean variable (or CancellationToken) and first thing my callback checks is whether its still supposed to run.

[KathleenDollard]

I think we should go forward with this. Sorry again for the delay.

[RussKie]

Setting as up for grabs.