-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue #4877
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
Conversation
|
|
||
| struct ReactDispatcher : implements<ReactDispatcher, IReactDispatcher> { | ||
| ReactDispatcher() = default; | ||
| ReactDispatcher(Mso::DispatchQueue &&queue) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DispatchQueue [](start = 23, length = 13)
(nit) why not IDispatchQueue? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The open source implementation of the DispatchQueue is different. We do not use the interface directly anymore. Instead, the DispatchQueue is a wrapper of the interface similar to what we do with the Mso::Future or Mso::Promise.
The good thing about having a wrapper class like Mso::DispatchQueue is that we can have template methods there, static methods, and all the DispatchQueue APIs are grouped under one class that makes it IntelliSense friendly.
In reply to: 423963640 [](ancestors = 423963640)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the mechanism you describe above allow people to provide their own dispatcher implementations?
In reply to: 423982263 [](ancestors = 423982263,423963640)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new Mso::DispatchQueue model we have only one Mso::DispatchQueue class that manages queue of tasks. Then we have IDispatchQueueScheduler interface that can run these tasks according to specific rules on the threads it wants. It is relatively small interface and developers can provide their own implementation of it. By implementing custom IDispatchQueueScheduler interface, developers can implement a 'custom queue' when they give it to the Mso::DispatchQueue constructor.
In reply to: 423983579 [](ancestors = 423983579,423982263,423963640)
|
|
||
| static Mso::DispatchQueue GetUIDispatchQueue(IReactPropertyBag const &properties) noexcept; | ||
|
|
||
| static IReactDispatcher UIThreadDispatcher() noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIThreadDispatcher [](start = 26, length = 18)
Should this match api on line 23? Eg: GetUIThreadDispatcher? or the other way around? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are tow different APIs:
- UIThreadDispatcher returns IReactDispatcher associated with the current UI thread.
- GetUIThreadDispatcher is an 'attached property' method that returns UIThreadDispatcher stored in the property bag.
In reply to: 423966470 [](ancestors = 423966470)
|
|
||
| static Mso::DispatchQueue GetUIDispatchQueue(IReactPropertyBag const &properties) noexcept; | ||
|
|
||
| static IReactDispatcher UIThreadDispatcher() noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IReactDispatcher [](start = 9, length = 16)
(nit) shouldn't this be a pointer? Or winrt idl is smart for this type of object? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++/WinRT interface-looking types are actually smart pointers to a real ABI interface.
In reply to: 423966942 [](ancestors = 423966942)
| ReactDispatcher(Mso::DispatchQueue &&queue) noexcept; | ||
|
|
||
| bool HasThreadAccess() noexcept; | ||
| void Post(ReactDispatcherCallback const &callback) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
& [](start = 42, length = 1)
Why not &&? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is C++/WinRT thing : they pass parameters by const&. I will check again if we can use && in places like this, but my previous tries were not successful.
In reply to: 423967144 [](ancestors = 423967144)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it. It does not compile. The C++/WinRT uses const&.
In reply to: 423983290 [](ancestors = 423983290,423967144)
| static runtimeclass ReactDispatcherHelper | ||
| { | ||
| // Get or create IReactDispatcher for the current UI thread. | ||
| static IReactDispatcher UIThreadDispatcher{ get; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this exposed to consumers? From what I understood we weren't sure yet how we wanted to treat the multi-UI-thread scenario. I worry that anything we do expose would be hard to walk back. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I misinterpreted the API. If this is just for getting the dispatcher of current thread that makes sense. Questions above still apply to UIThreadDispatcherProperty though. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is per thread. It does not matter what we decide in future: this code will work in any case. There is no such thing as 'perfect API'. This design works today. Tomorrow if we have new requirements we will change it.
In reply to: 423970645 [](ancestors = 423970645)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is about getting dispatcher of the current thread. The UIThreadDispatcherProperty can be used to set the UIThreadDispatcher in the ReactPropertyBag.
In reply to: 423981509 [](ancestors = 423981509)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vmoroz are external native modules expected to use this? "Tomorrow if we have new requirements we will change it." becomes painful if users start relying on this. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current vision is that developers of native modules can access IReactDispatcher of current UI thread using this API. This is a new API. I cannot give you 100% guarantee that this is the final one. It must tested by real usage first. I do not believe we any new API where we can provide such guarantee. I am not sure what is your concern here. Did you ever create API that you never changed?
In reply to: 423993539 [](ancestors = 423993539)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course we will sometimes need to make changes. It's just much more expensive to do this for the public API of a package where we don't control consumers.
For internal code, or if we're in a monorepo setup like Office, we can just fixup old code using an API we don't like. It's much more expensive to do this for RNW. We either make sudden breaks, which has been a big cause of customer frustration, or we need to do phased deprecation, which is a but less painful, but annoying to track and maintain.
In this instance, we know we have the potential to run into some issues with the current design. It feels like we could avoid pain if we can prevent these rapid changes. E.g. expose this internally for now.
Consumers can still manually inject a queue of their own property if they're okay with it being instance global. Directing people towards that would avoid needing to deal with breaks and user frustration if we decide we can do something better. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"In this instance, we know we have the potential to run into some issues with the current design." Could you please share what you see as an issue?
Previous design was not multi UI thread friendly. This one makes it more friendly, but we still must provide a dedicated UI thread per RNH. It is not a 'true' multi-UI concept, but we must do it today because many native components and RN itself needs it. So, as any other engineering solution we must implement some kind of 'transitional' design. But even if multi-UI will never happen this current design is still much better and more usable comparing to what we have today.
I would like to understand what type of design issue you see that makes you block this PR.
In reply to: 424009257 [](ancestors = 424009257)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure how long-term multi-ui thread was. From the last discussion I was under the impression there was some active effort there, but now it sounds like it's further out. Just wanting to make sure we don't break this too quickly or box ourselves into a corner.
Signing off. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the sign off!
As we discussed offline before, the core issue with the multi-UI threads are the core native modules and the core RN that relies on the single UI thread.
This change allows us to bring the UI queue to the RNH as an external entity. The RNH does not create it anymore. This design change moves us closer to the multi-UI threads if we ever do it. Plus, it enables use of the RNH for the XAML islands.
In reply to: 424022761 [](ancestors = 424022761)
| static IReactDispatcher UIThreadDispatcher{ get; }; | ||
|
|
||
| // Get name of the UIThreadDispatcher property. | ||
| static IReactPropertyName UIThreadDispatcherProperty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Not sure we should expose what we consider to be the UI thread yet. People can already do this manually with the property bag, right? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do want to directly expose a thread to the consumers and its available in all cases, maybe we should expose it on the context + InstanceSettings without letting people know it's in the bag? From the outside observer perspective, ReactHost in Office using the property bag for queues felt like it was hiding something meant to be explicit. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We may do it in future. For now I would prefer it to be in the property bag. There are several reasons:
- We want to associate a queue with a native module. The best way to do it is to associate property ID with the native module. This property ID can be used to retrieve the real queue before we make instance of the module. It is the main reason why we are doing it in Office for Reka services.
- I do not want the ReactContext become too big or too fat. E.g. today it depends on XAML for view events and it already creates a lot of issues. Ideally, the context must be as small as possible. Its main goal is to be a central point to access shared data and resources from native modules. Ideally no preference must be given to any specific interface or component.
In reply to: 423973960 [](ancestors = 423973960)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There must be a way to put something in the property bag. In simple UWP 'green' applications we already do it in the ReactApplication. But for win32 XAML islands, we cannot use it and we must provide ability:
- What to put as a UI dispatcher in the property bag - UIThreadDispatcher property.
- How to put it in the property bag - use the UIThreadDispatcherProperty to store the UIThreadDispatcher.
In reply to: 423971383 [](ancestors = 423971383)
| return IDispatchQueueStatic::Instance()->ConcurrentQueue(); | ||
| } | ||
|
|
||
| inline /*static*/ DispatchQueue const &DispatchQueue::MainUIQueue() noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MainUIQueue [](start = 54, length = 11)
Isn't this a break from DevMain? Would DevMain use new pattern in the future? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is not in the DevMain. It is part of the experimental DispatchQueue 2.0 implementation. At some point I thought that it is a great idea to have such global property, but not any more because we have scenarios in RNW where it does not work.
So, this experimental feature will never land to DevMain.
In reply to: 423973832 [](ancestors = 423973832)
NikoAri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
| <ClInclude Include="ReactHost\UwpReactInstanceProxy.h"> | ||
| <Filter>ReactHost</Filter> | ||
| </ClInclude> | ||
| <ClInclude Include="ReactPropertyBag.h" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was removed automatically by VS. We do not have such files. The implementation files are IReactPropertyBag.h and IReactPropertyBag.cpp. I never can understand what rules VS uses to change the .filters files.
In reply to: 423977037 [](ancestors = 423977037)
NickGerleman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to know more about what we're planning with the public API changes here.
| delegate void ReactDispatcherCallback(); | ||
|
|
||
| [webhosthidden] | ||
| interface IReactDispatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this replace the IMessageQueue interface (see Desktop/ABI/MessageQueue.idl)? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static runtimeclass ReactDispatcherHelper | ||
| { | ||
| // Get or create IReactDispatcher for the current UI thread. | ||
| static IReactDispatcher UIThreadDispatcher{ get; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get [](start = 48, length = 3)
For my understanding, who sets this? Does the API design represented here accommodate user who want to provide there own dispatcher implementations? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a shortcut to get a default implementation out of the current UI thread.
The current design has a gap that I do not understand yet how we can supply custom Office implementation. We must go back to solve it.
E.g. one of the solutions could be to make this property settable to enable such scenarios.
In reply to: 423982313 [](ancestors = 423982313)
|
The main goal at this point is to supply UI dispatch queue from outside of ReactNativeHost. The ReactNativeHost cannot create it on its own because it may be created in a background thread. Thus, the UI thread must be passed explicitly. In reply to: 410353268 [](ancestors = 410353268) |
|
|
||
| // Helper methods for the property bag implementation. | ||
| [webhosthidden] | ||
| static runtimeclass ReactDispatcherHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static runtimeclass ReactDispatcherHelper [](start = 2, length = 41)
Nit: Should we keep it at one type per file? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we do not have such requirement. Seeing how slow the MIDL compiler is, I would say the opposite: less IDL files we have is better. I would rather use the IDL files as a group of definitions related to some functionality.
In reply to: 423987808 [](ancestors = 423987808)
| static const char *ToString(ApplicationTheme theme) noexcept; | ||
| void OnColorValuesChanged() noexcept; | ||
|
|
||
| private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private [](start = 1, length = 7)
Nit: Why the repetition of the 'private:' marker? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we leave this as is? I prefer the way I originally wrote this, and don't see why we should add our preferences to other code. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aeulitz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
| using namespace winrt; | ||
| using namespace Windows::Foundation; | ||
|
|
||
| namespace winrt::Microsoft::ReactNative { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why prefix these namespaces winrt::? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The winrt:: prefix is commonly used for the C++/WinRT components. It is used by default by the C++/WinRT code generator. It is much easier to author our own components in that namespace. Plus, we can use many winrt functions without prefixes.
Do you think it would be preferable not to use it?
In reply to: 424007290 [](ancestors = 424007290)
|
Hello @vmoroz! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 hours, a condition that will be fulfilled in about 7 hours 17 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…icrosoft#4877) * Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue * Change files * Removed 'Thread' from some new API names. * Removed extra 'private' section in AppearanceModule * Temporary: use CoreDispatcher to check the E2E test * Avoid using DispatcherQueue::HasThreadAccess in earlier Windows versions.
…icrosoft#4877) * Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue * Change files * Removed 'Thread' from some new API names. * Removed extra 'private' section in AppearanceModule * Temporary: use CoreDispatcher to check the E2E test * Avoid using DispatcherQueue::HasThreadAccess in earlier Windows versions.
…4877) * Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue * Change files * Removed 'Thread' from some new API names. * Removed extra 'private' section in AppearanceModule * Temporary: use CoreDispatcher to check the E2E test * Avoid using DispatcherQueue::HasThreadAccess in earlier Windows versions.
* improve treedump capabilities * Reenable V8 for desktop projects (#4840) * Delay load ChakraCore.dll * Change files * clean up unneeded project references * Overwrite property after React.Cpp.props gets included * Change files * change file * Fix WinUI3 * Reverse the conditional logic * Revert some unnecessary changes. Consolidate the preprocessor defines in the props Co-authored-by: tudorm <[email protected]> * applying package updates ***NO_CI*** * update masters * Improve run_wdio to print out failed tests (#4843) * Improve run_wdio to print out failed tests * Fire onLoad event when a bitmap image is opened (#4750) * Fire onLoad event when a bitmap image is opeed * Change files * add imageFailed * remove firing of onload events in the createImageBrush block so that load events don't fire twice. * Expose ability for apps to provide their own RedBox implementation (#4786) * Expose ability for apps to provide their own RedBox implementation * Change files * minor changes * Code review feedback * minor fix * format fixes * minor change * minor fix * minor fix * minor fix * Bump fp-ts from 2.5.4 to 2.6.0 (#4871) Bumps [fp-ts](https://github.com/gcanti/fp-ts) from 2.5.4 to 2.6.0. - [Release notes](https://github.com/gcanti/fp-ts/releases) - [Changelog](https://github.com/gcanti/fp-ts/blob/master/CHANGELOG.md) - [Commits](gcanti/fp-ts@2.5.4...2.6.0) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> * applying package updates ***NO_CI*** * Fixed ReactContext copy/move semantic (#4870) * Fixed ReactContext copy/move semantic * Change files * Fixed formatting * Test to show how to use a delegate in ReactPropertyBag * Add ReactNativeHost to Win32 C++/WinRT ABI (#4848) * create MS.ReactNative.IntegrationTests project * RNHost activation succeeds * update * update * Change files * add JS function call test * Change files * fix formatting * submit current state * ReactNativeHost activation test succeeds * Change files * account for property bag addition * sync, address code review feedback * Use spec file for DevSettings NativeModule (#4873) * Use spec file for DevSettings NativeModule * Change files * minor change * Handle HTTP errors in DevSupportManager. (#4880) * Handle HTTP connect exceptions in DevSupportManager * Change files * applying package updates ***NO_CI*** * Allow storing non-WinRT types in ReactPropertyBag (#4884) * Allow storing non-WinRT types in ReactPropertyBag * Change files * Addressed PR feedback * Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue (#4877) * Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue * Change files * Removed 'Thread' from some new API names. * Removed extra 'private' section in AppearanceModule * Temporary: use CoreDispatcher to check the E2E test * Avoid using DispatcherQueue::HasThreadAccess in earlier Windows versions. * RNW dependencies (#4876) * add rnw-dependencies.ps1 * Change files * --sln * run in CI * print * desktop appium is optional * use agent directory * 15GB is a ballpark estimate * optional space * . * support running from web (#4892) * Bump typescript from 3.8.3 to 3.9.2 (#4890) * Bump lerna from 3.20.2 to 3.21.0 (#4889) * BugFix: fix default tabindex stomping over acceptsKeyboardFocus (#4893) * Change files * revert dfc57fc * set default tab index to -1, the hope is to be in line with xbox assumption * xbox doesn't seem to rely on this, changing default back to match xaml default * Update e2e testing doc with CI debugging info (#4897) * add instructions for CI debugging for e2e test app * point e2e readme to e2e testing doc * Update rnw-dependencies (#4894) * don't exit the powershell session and pause for keyboard input if interactive * Change files * enable Switch (fixed 4596) * improve treedump capabilities * update masters * enable Switch (fixed 4596) * protect against exceptions in run_wdio * add another try block * update e2e testing and masters, fixes 4680 * publish wdio report and fix run_wdio typo bug * TreeDump should ignore collapsed object elements in an array * add info about run_wdio Co-authored-by: tudorms <[email protected]> Co-authored-by: tudorm <[email protected]> Co-authored-by: React-Native-Windows Bot <[email protected]> Co-authored-by: lamxdoan <[email protected]> Co-authored-by: Andrew Coates <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Vladimir Morozov <[email protected]> Co-authored-by: Andreas Eulitz <[email protected]> Co-authored-by: Julio César Rocha <[email protected]> Co-authored-by: kmelmon <[email protected]>
The CoreDispatcher is a UWP-only implementation of a queue running on top of a window message handler. The newer Windows versions have new DispatcherQueue that works for UWP and Win32 applications.
In this PR we change the Mso::DispatchQueue to use the new DispatcherQueue instead of CoreDispatcher. It enables use of the UI Mso::DispatchQueue implementation on Win32 platforms.
We had to change use of the UI Mso::DispatchQueue in our code to accommodate the DispatcherQueue restriction that it cannot be globally discovered in the application. Instead, we must create it in the ReactApplication and pass through the ReactNativeHost.InstanceSettings().Properties() to other components and native modules.
After this change we do not see the Playground Win32 crash at the startup.
Microsoft Reviewers: Open in CodeFlow