Winforms: TaskDialog.ShowDialog() doesn't install WindowsFormsSynchronizationContext

[kpreisser]

• .NET Core Version:
  5.0.100-preview.5.20255.2

• Have you experienced this same bug with .NET Framework?:
  No

Problem description:

Currently, TaskDialog.ShowDialog() doesn't install the WindowsFormsSynchronizationContext, so when showing the task dialog from a thread that didn't call a Control constructor and which currently isn't running a message loop e.g. with Form.ShowDialog() or Application.Run(), continuations of async methods that were initiated from an task dialog event won't run on the same thread used to show the task dialog.

Expected behavior:

I think TaskDialog.ShowDialog should install the WindowsFormsSynchronizationContext before actually showing the dialog if SynchronizationContext.Current isn't an WindowsFormsSynchronizationContext, and in that case, should also uninstall it after the native TaskDialogIndirect returns.

Note: It seems MessageBox.Show() also doesn't install the WindowsFormsSynchronizationContext, but there it's probably not that important since yout normally don't handle events within MessageBox.Show().

What do you think?

Minimal repro:
```c#
[STAThread]
static void Main()
{
Application.EnableVisualStyles();
var mainThread = Thread.CurrentThread;

var myPage = new TaskDialogPage();
myPage.Created += async (s, e) =>
{
    Console.WriteLine("TaskDialog.ShowDialog():");
    Console.WriteLine("Is WindowsFormsSynchronizationContext: " +
        (SynchronizationContext.Current is WindowsFormsSynchronizationContext));

    await Task.Yield();

    Console.WriteLine("Is Main Thread: " + (Thread.CurrentThread == mainThread));
    Console.WriteLine();
    myPage.BoundDialog!.Close();
};
TaskDialog.ShowDialog(myPage);

using var myForm = new Form();
myForm.Load += async (s, e) =>
{
    Console.WriteLine("Form.ShowDialog():");
    Console.WriteLine("Is WindowsFormsSynchronizationContext: " +
        (SynchronizationContext.Current is WindowsFormsSynchronizationContext));

    await Task.Yield();

    Console.WriteLine("Is Main Thread: " + (Thread.CurrentThread == mainThread));
    Console.WriteLine();
    myForm.Close();
};
myForm.ShowDialog();

}

**Actual output:**

TaskDialog.ShowDialog():
Is WindowsFormsSynchronizationContext: False
Is Main Thread: False

Form.ShowDialog():
Is WindowsFormsSynchronizationContext: True
Is Main Thread: True

**Expected output:**

TaskDialog.ShowDialog():
Is WindowsFormsSynchronizationContext: True
Is Main Thread: True

Form.ShowDialog():
Is WindowsFormsSynchronizationContext: True
Is Main Thread: True
```

[weltkante]

Please proceed very carefully here, the design of the WinForms message loop is very complex because it integrates with Office and its still up for discussion whether this support is still needed or going to be removed. Installing a WinFormsSynchronizationContext is _not_ enough to have a proper message loop integration.

It seems MessageBox.Show() also doesn't install the WindowsFormsSynchronizationContext

It shouldn't need to, it should be forwarding to the win32 implementation which has its own message loop. Message boxes are a very special beast and you need to be able to call them without message loop. Installing a SC is bad because someone may capture it and then not being able to get back to it once the message box is closed.

continuations of async methods that were initiated from an task dialog event won't run on the same thread used to show the task dialog.

Installing a SC won't fix that, that will just deadlock, since the message loop is no longer running and the SC is dead. If I remember right the WinForms SC is not built for repeated install/uninstall scenarios and will perform poorly, it does not have a good shutdown experience and async tasks exceeding the message loop will not deal gracefully with another SC coming back to live in a different message loop.

I think TaskDialog.ShowDialog should install the WindowsFormsSynchronizationContext before actually showing the dialog if SynchronizationContext.Current isn't an WindowsFormsSynchronizationContext, and in that case, should also uninstall it after the native TaskDialogIndirect returns.

Please do whatever Form.ShowDialog does when being run without a message loop. Do not invent any new schemes, this will just introduce new hard to diagnose bugs.

If thats not possible you should throw an exception and let the user set up a message loop. Or just accept the situation that TaskDialog is like MessageBox and can be driven without an external SC.

[kpreisser]

Hi @weltkante ,

thank you for the insights.

Installing a SC is bad because someone may capture it and then not being able to get back to it once the message box is closed.

OK, agreed.

continuations of async methods that were initiated from an task dialog event won't run on the same thread used to show the task dialog.

Installing a SC won't fix that, that will just deadlock, since the message loop is no longer running and the SC is dead.

Can you elaborate on that? When I insert a call to new Control(); at the beginning of the Main method shown in my example (which also installs the synchronization context), then the console shows the expected behavior (continuation of the async method is run in the GUI thread), because while the task dialog is showing, it runs its own message loop and will run callbacks scheduled by the WindowsFormsSynchronizationContext.

Are you referring to the case where an async method's continuation is to be called after the task dialog has already closed? In that case, yes, the continuation will not be called if there is no outer message loop to continue.
However, that's the same case as when calling Form.ShowDialog() (without an outer message loop), as when Form.ShowDialog() returns, the message loop has ended, and it also uninstalls the synchronization context. (But uninstalling the context doesn't seem to dispose of the MarshalingControl, so when a message loop is run again, e.g. by calling Application.Run(), the continuations will still be run.)

If thats not possible you should throw an exception and let the user set up a message loop. Or just accept the situation that TaskDialog is like MessageBox and can be driven without an external SC.

OK, thanks. I don't know much about how the WinForms message loop is implemented internally, so I don't know what's the best action to do here - maybe TaskDialog.ShowDialog() shouldn't set up a ExecutionContext then.
(My motivation was that a user might expect the code of async functions called in task dialog event handlers to always run in the GUI thread while showing the task dialog, even when there is no outer message loop.)

Thank you!

[weltkante]

Are you referring to the case where an async method's continuation is to be called after the task dialog has already closed?

Yes that was what I was referring to, even if you try writing something with cancellation tokens you'll have a hard time properly terminating async operations because once you leave the ShowDialog your async calls are hanging, so its too late for cancellation. You have to cancel _and wait for completion_ before leaving a message loop.

SC has OperationStarted/OperationCompleted callbacks to count outstanding async operations and let them drain before shutdown, but WinForms never implemented this because the intention is that once the message loop terminates the application also terminates.

However, that's the same case as when calling Form.ShowDialog()

Yes I was arguing two separate things:
1) this is a problem that isn't simple to solve, uninstalling an SC will cause other problems (deadlocks, dead SC instances) so you just shifted the problem with async methods, not solved it
2) Form.ShowDialog is the precedent how to install a temporary message loop, inventing new ways will not integrate correctly with Office (should it be decided to continue supporting that)

OK, thanks. I don't know much about how the WinForms message loop is implemented internally

ThreadContext is the core component here, it coordinates everything, including the Office interop via IMsoComponent, but there are also managed hooks for when a modal loop starts/ends.

I don't know how well it fits when win32 TaskDialog is driving the modal loop instead of WinForms.

A compromise would be calling WinFormsSynchronizationContext.InstallIfNeeded (basically what the Control constructor does when invoked on a thread which isn't a UI thread yet). This would not integrate well with Office but at least not introduce any new patterns to the already complex logic. But I wouldn't uninstall because that causes a ton of problems you can't easily solve.

I'd recommend the following approach:

• research whether TaskDialog can be integrated with ThreadContext, to simulate what Form.ShowDialog does.
• if not possible consider WFSC.InstallIfNeeded to simulate what a Control constructor does (do the work equivalent to a using(new Control()) { }, might be more than just installing SC)
• alternatively treat it like MessageBox and do nothing (assume the user is responsible for setting up the message loop if he needs it)

[weltkante]

I've thought about it and think I like option (2) from my previous post most currently. The TaskDialog constructor -not ShowDialog- should do the same work the Control constructor does, installing a WFSC by calling InstallIfNeeded (but not uninstalling it once its done, since Controls don't do that either).

This strategy of treating it like a Control as far as WinForms initialization is concerned should not introduce any new behavior and keep the risk minimal:

• creating Controls is not a promise to show them, you could dispose them without showing them; similar, creating a TaskDialog is not a promis to actually show it
• creating Controls on a thread before the message loop is installed is normal, you run the Form constructor with all Controls before running the Application message loop or a top level Form.ShowDialog

this makes TaskDialog just another variant of the existing code paths.

It doesn't solve the shutdown problems but I think those are out of scope of this issue, if someone is to approach WFSC shutdown to make it more async-friendly it needs to be its own issue and be done across whole WinForms and not just one usecase like here.

Office integration can be treated similar to MessageBox, since (I assume, didn't check) the message loop of TaskDialog is win32 and not WinForms it might be hard to negotiate the IMsoComponent state.

Sorry if I'm ranting a bit, got surprised by this issue and I've not got the time to think everything through before I wrote my first posts.

[RussKie]

May be do nothing and let user to facilitate a correct SC?
In Git Extensions we do this for the same reason:
https://github.com/gitextensions/gitextensions/blob/master/GitExtensions/Program.cs#L79-L84

How general do you think this use case where a user spawns a TD outside/absent of a form and then starts performing async operations?

Right now to me this feels like an edge case that is best handled via documentation. I.e. document it as "_don't do it at home, but if you really want to do it - ensure you have SC_".

[kpreisser]

Hi @weltkante and @RussKie,

thank you for your replies.

I have thought a bit more about this, and I think I agree with @RussKie that it's probably better not to install the WFSC in the TaskDialog, but rather only document this; so that TaskDialog doesn't behave differently than MessageBox in this regard.
Otherwise, I could imagine that users might want to show a MessageBox in different (non-UI) threads where they also call async methods, and then replace MessageBox.Show() with TaskDialog.ShowDialog(), and suddenly the async task continuations started on that thread won't be executed any more after the TaskDialog is closed, due to the installed WFSC, which was not the case when using MessageBox.

I initially raised this issue as recently I changed the multi-page task dialog example in the TaskDialogDemo project (and then also the sample code in #146) to use an async method rather than a Timer to simulate the background operation, as I think that comes closer to what a real-world implementation might look like, and the code should be easier to follow.
But then I realized that when copy-pasting the example code into a console application (which is what I often do to test the task dialog) without other WinForms code, the continuations of the async method (and therefore the code that updates the TaskDialog) might be executed in a worker thread rather than the main thread due to the missing SynchronizationContext. (But the example code can be modified to ensure the WindowsFormsSynchronizationContext is installed before showing the dialog.)

Regarding the different thread, note that while it sometimes may work to update the task dialog from a different thread (as it calls SendMessage which blocks until the GUI thread processed the message), the TaskDialog implementation is not thread-safe, and if one thread modifies the task dialog but at the same time the GUI thread processes user input from the message loop, race conditions can occur and the resulting state will be unpredictable.
Maybe we should add checks that the TaskDialog (e.g. the Handle property) is only accessed from the same thread which created the window, as a best-effort mechanism to make the user aware of the incorrect thread usage, similar to the Control.CheckForIllegalCrossThreadCalls mechanism.

What do you think?

Thanks!

[weltkante]

I don't have any objections about leaving the SC installation to the user and documenting it.

Maybe we should add checks that the TaskDialog (e.g. the Handle property) is only accessed from the same thread which created the window, as a best-effort mechanism to make the user aware of the incorrect thread usage, similar to the Control.CheckForIllegalCrossThreadCalls mechanism.

You can try doing that. Is TaskDialog handle based or a COM API? If its COM just checking the thread won't be enough. Depending on how the COM classes are configured TaskDialog may reject running on a MTA thread and spin up a STA threadpool thread just for running the TaskDialog. Normally this happens transparently, but if you are going to check threads of handles it may mess with your checks. Just dropping that as a note to be aware of if you are attempting to do thread checks.

[kpreisser]

Is TaskDialog handle based or a COM API?

AFAIK, TaskDialog is not a COM API. It's a regular function TaskDialogIndirect in comctl32.dll.

(The documentation says that TaskDialog requires the single-threaded apartment (STA) model, but I didn't experience any problems yet when showing the task dialog from threads using the MTA model. When I call GetWindowThreadProcessId with the task dialog handle from an MTA thread, it returns the same value as GetCurrentThreadId.)

Thank you!

[merriemcgaw]

We need to document that this isn't supported (update docs). We should also throw in this scenario.

[RussKie]

@gewarren the TaskDialog API have come in P4. I don't believe the docs from P4 have been published yet. What's the best way for us to add/update the docs for the TaskDialog API?

[gewarren]

@gewarren the TaskDialog API have come in P4. I don't believe the docs from P4 have been published yet. What's the best way for us to add/update the docs for the TaskDialog API?

@russkie Bill @BillWagner has a PR to update the docs for Preview 3 that I'll approve and merge this afternoon. We can create a similar PR for Preview 4. When does that release?

[BillWagner]

I have the binaries to start the process for Preview 4. We'll publish those in coordination with the Preview 4 release.

[paul1956]

@RussKie does the statement from @BillWagner change your answer about what I need to run the demo? If not I am missing something getting the demo working.

[RussKie]

We should also throw in this scenario.

@kpreisser could you amend the code to throw for x-thread access?
We'll update the docs when P5 opens.

[msftbot[bot]]

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

[RussKie]

@kpreisser do you think you could send a PR that checks for x-thread access?

[kpreisser]

Sorry for the delay. Yes, I think I can send a PR for the cross-thread access check.

[merriemcgaw]

3417 addresses the issue. Closing this.