Wrapped UI dependant calls in WindowsProvider with dispatchers #136
Conversation
|
Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
| @@ -197,7 +198,21 @@ public override async Task<string> GetTokenAsync(bool silentOnly = false) | |||
| } | |||
|
|
|||
| // Attempt to authenticate interactively. | |||
| authResult = await AuthenticateInteractiveAsync(_scopes); | |||
| var tcs = new TaskCompletionSource<WebTokenRequestResult>(); | |||
| var taskQueued = DispatcherQueue.GetForCurrentThread().TryEnqueue(async () => | |||
There was a problem hiding this comment.
GetForCurrentThread gets the current thread still. So if SignInAsync isn't called on the UI Thread, this won't dispatch back to the UI Thread.
Instead, you want to grab a reference to the DispatcherQueue of the UI thread, for instance if you know WindowsProvider will always be constructed from the UI of the application, grab it in the constructor. Then when you use it here, it'll dispatch to that original UI thread vs. whatever thread SignInAsync is called from.
Think I got that right @azchohfi?
There was a problem hiding this comment.
Can I actually safely assume that the WindowsProvider will be constructed from the UI thread? Or I guess I'd be enforcing that it be constructed from the UI thread by accepting the constructor param, right?
There was a problem hiding this comment.
I mean that's usually a documentation thing like 'you must construct the provider on the UI thread' but you can also just accept the DispatcherQueue as a constructor parameter. @azchohfi did that in a few cases for the Toolkit I think.
For controls we always know the constructor is called on the UI thread as the XAML engine is creating it. Here, as long as they're creating the provider in their App it should as well I think?
There was a problem hiding this comment.
Yeah, I would create a test for this to make sure we are covering all the cases. I don't believe we can force every developer to create this from the UI thread (using it in XAML), right?
You can expose a DispatcherQueue property that is automatically populated in the ctor, if it finds one. If it doesn't, then when you try to use it and it is null, throw an exception to tell the developer that they have to either call the ctor in the ui thread, or manually set the dispatcher once they have it. Make it a get;set; prop, not readonly. Does that make sense? I believe I followed this pattern in the Xaml Islands compat pass that I did a few years ago.
There was a problem hiding this comment.
I think this is the pattern Alex is talking about:
(ugh really need GitHub to inline code-snippets across repositories)
I didn't see us checking though, just either taking the one in or grabbing the current thread, doc comment calls out constructing on UI thread. I don't think we have an explicit way to be able to determine if a particular queue is the 'ui' one, eh?
There was a problem hiding this comment.
I don't think we have an explicit way to be able to determine if a particular queue is the 'ui' one, eh?
Not that I know of 🤔 But thanks for the suggestion @azchohf, and thanks for the links @michael-hawker! I can totally implement that 😁
There was a problem hiding this comment.
I am curious to know what's the problem in using CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync:
await CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(
Windows.UI.Core.CoreDispatcherPriority.Normal, async () =>
{
var result = await AuthenticateInteractiveAsync(_scopes);
tcs.SetResult(result);
});You want to avoid referencing CoreWindow for some reason?
There was a problem hiding this comment.
Yes, we want to avoid using CoreWindow. CoreWindow have a concept of single instance. It is literally a singleton. The problem with it is that if you have multiple windows in your app (running in different threads), then you can't know which Dispatcher to dispatch to. This is particularly an issue with Win32, meaning Xaml Islands and WinUI3. The DispatcherQueue was literally built to fix this issue. It works on both UWP and Win32 contexts.
There was a problem hiding this comment.
OK, thanks for the clarification. While waiting for your fix I fixed this locally using CoreWindow, but it is an UWP app.
Fixes #132
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, UI based calls in the WindowsProvider are not protected in anyway. Causing occasional failures when called from a non-UI thread.
What is the new behavior?
The UI dependant calls in WindowsPRovider are now wrapped with calls to
DispatcherQueue.GetForCurrentThread().TryEnqueue(...);preventing threading errors regardless of calling location.PR Checklist
Please check if your PR fulfills the following requirements:
Other information