Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue #4877
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue", | ||
| "packageName": "react-native-windows", | ||
| "email": "[email protected]", | ||
| "dependentChangeType": "patch", | ||
| "date": "2020-05-12T18:18:15.956Z" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #include "pch.h" | ||
| #include "IReactDispatcher.h" | ||
| #include "ReactDispatcherHelper.g.cpp" | ||
|
|
||
| using namespace winrt; | ||
| using namespace Windows::Foundation; | ||
|
|
||
| namespace winrt::Microsoft::ReactNative { | ||
|
|
||
| ReactDispatcher::ReactDispatcher(Mso::DispatchQueue &&queue) noexcept : m_queue{std::move(queue)} {} | ||
|
|
||
| bool ReactDispatcher::HasThreadAccess() noexcept { | ||
| return m_queue.HasThreadAccess(); | ||
| } | ||
|
|
||
| void ReactDispatcher::Post(ReactDispatcherCallback const &callback) noexcept { | ||
| return m_queue.Post([callback]() noexcept { callback(); }); | ||
| } | ||
|
|
||
| /*static*/ Mso::DispatchQueue ReactDispatcher::GetUIDispatchQueue(IReactPropertyBag const &properties) noexcept { | ||
| return GetUIDispatcher(properties).as<ReactDispatcher>()->m_queue; | ||
| } | ||
|
|
||
| /*static*/ IReactDispatcher ReactDispatcher::UIThreadDispatcher() noexcept { | ||
| return make<ReactDispatcher>(Mso::DispatchQueue::MakeCurrentThreadUIQueue()); | ||
| } | ||
|
|
||
| /*static*/ ReactPropertyId<IReactDispatcher> ReactDispatcher::UIDispatcherProperty() noexcept { | ||
| static ReactPropertyId<IReactDispatcher> uiThreadDispatcherProperty{L"ReactNative.Dispatcher", L"UIDispatcher"}; | ||
| return uiThreadDispatcherProperty; | ||
| } | ||
|
|
||
| /*static*/ IReactDispatcher ReactDispatcher::GetUIDispatcher(IReactPropertyBag const &properties) noexcept { | ||
| return ReactPropertyBag{properties}.Get(UIDispatcherProperty()); | ||
| } | ||
|
|
||
| /*static*/ void ReactDispatcher::SetUIThreadDispatcher(IReactPropertyBag const &properties) noexcept { | ||
| ReactPropertyBag{properties}.Set(UIDispatcherProperty(), UIThreadDispatcher()); | ||
| } | ||
|
|
||
| } // namespace winrt::Microsoft::ReactNative | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #pragma once | ||
| #include "ReactDispatcherHelper.g.h" | ||
| #include <ReactPropertyBag.h> | ||
| #include <dispatchQueue/dispatchQueue.h> | ||
| #include <winrt/Microsoft.ReactNative.h> | ||
|
|
||
| namespace winrt::Microsoft::ReactNative { | ||
|
|
||
| struct ReactDispatcher : implements<ReactDispatcher, IReactDispatcher> { | ||
| ReactDispatcher() = default; | ||
| ReactDispatcher(Mso::DispatchQueue &&queue) noexcept; | ||
|
There was a problem hiding this comment.
(nit) why not IDispatchQueue? #Resolved
Member
Author
There was a problem hiding this comment. 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. In reply to: 423963640 [](ancestors = 423963640)
Contributor
There was a problem hiding this comment. Does the mechanism you describe above allow people to provide their own dispatcher implementations? In reply to: 423982263 [](ancestors = 423982263,423963640)
Member
Author
There was a problem hiding this comment. 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) |
||
|
|
||
| bool HasThreadAccess() noexcept; | ||
| void Post(ReactDispatcherCallback const &callback) noexcept; | ||
|
There was a problem hiding this comment.
Why not &&? #Resolved
Member
Author
There was a problem hiding this comment. 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)
Member
Author
There was a problem hiding this comment. I tried it. It does not compile. The C++/WinRT uses const&. In reply to: 423983290 [](ancestors = 423983290,423967144) |
||
|
|
||
| static Mso::DispatchQueue GetUIDispatchQueue(IReactPropertyBag const &properties) noexcept; | ||
|
|
||
| static IReactDispatcher UIThreadDispatcher() noexcept; | ||
|
There was a problem hiding this comment.
Should this match api on line 23? Eg: GetUIThreadDispatcher? or the other way around? #Resolved
Member
Author
There was a problem hiding this comment. These are tow different APIs:
In reply to: 423966470 [](ancestors = 423966470) There was a problem hiding this comment.
(nit) shouldn't this be a pointer? Or winrt idl is smart for this type of object? #Resolved
Member
Author
There was a problem hiding this comment. C++/WinRT interface-looking types are actually smart pointers to a real ABI interface. In reply to: 423966942 [](ancestors = 423966942) |
||
| static ReactPropertyId<IReactDispatcher> UIDispatcherProperty() noexcept; | ||
| static IReactDispatcher GetUIDispatcher(IReactPropertyBag const &properties) noexcept; | ||
| static void SetUIThreadDispatcher(IReactPropertyBag const &properties) noexcept; | ||
|
|
||
| private: | ||
| Mso::DispatchQueue m_queue; | ||
| }; | ||
|
|
||
| } // namespace winrt::Microsoft::ReactNative | ||
|
|
||
| namespace winrt::Microsoft::ReactNative::implementation { | ||
|
|
||
| struct ReactDispatcherHelper { | ||
| ReactDispatcherHelper() = default; | ||
|
|
||
| static IReactDispatcher UIThreadDispatcher() noexcept { | ||
| return ReactDispatcher::UIThreadDispatcher(); | ||
| } | ||
|
|
||
| static IReactPropertyName UIDispatcherProperty() noexcept { | ||
| return ReactDispatcher::UIDispatcherProperty().Handle(); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace winrt::Microsoft::ReactNative::implementation | ||
|
|
||
| namespace winrt::Microsoft::ReactNative::factory_implementation { | ||
|
|
||
| struct ReactDispatcherHelper : ReactDispatcherHelperT<ReactDispatcherHelper, implementation::ReactDispatcherHelper> {}; | ||
|
|
||
| } // namespace winrt::Microsoft::ReactNative::factory_implementation | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import "IReactPropertyBag.idl"; | ||
|
|
||
| namespace Microsoft.ReactNative { | ||
|
|
||
| // The delegate is used to create property value on-demand. | ||
| [webhosthidden] | ||
| delegate void ReactDispatcherCallback(); | ||
|
|
||
| [webhosthidden] | ||
| interface IReactDispatcher | ||
|
Contributor
There was a problem hiding this comment. Can this replace the IMessageQueue interface (see Desktop/ABI/MessageQueue.idl)? #Resolved
Member
Author
There was a problem hiding this comment. |
||
| { | ||
| // True if dispatcher uses current thread. | ||
| Boolean HasThreadAccess { get; }; | ||
|
|
||
| // Post task for the asynchronous execution. | ||
| void Post(ReactDispatcherCallback callback); | ||
| } | ||
|
|
||
| // Helper methods for the property bag implementation. | ||
| [webhosthidden] | ||
| static runtimeclass ReactDispatcherHelper | ||
|
Contributor
There was a problem hiding this comment.
Nit: Should we keep it at one type per file? #Resolved
Member
Author
There was a problem hiding this comment. 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) |
||
| { | ||
| // Get or create IReactDispatcher for the current UI thread. | ||
| static IReactDispatcher UIThreadDispatcher{ get; }; | ||
|
Contributor
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. 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
Member
Author
There was a problem hiding this comment. 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)
Member
Author
There was a problem hiding this comment. 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)
Contributor
There was a problem hiding this comment. @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
Member
Author
There was a problem hiding this comment. 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)
Contributor
There was a problem hiding this comment. 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
Member
Author
There was a problem hiding this comment. "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? I would like to understand what type of design issue you see that makes you block this PR. In reply to: 424009257 [](ancestors = 424009257)
Contributor
There was a problem hiding this comment. 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
Member
Author
There was a problem hiding this comment. Thank you for the sign off! In reply to: 424022761 [](ancestors = 424022761)
Contributor
There was a problem hiding this comment.
For my understanding, who sets this? Does the API design represented here accommodate user who want to provide there own dispatcher implementations? #Resolved
Member
Author
There was a problem hiding this comment. This is just a shortcut to get a default implementation out of the current UI thread. In reply to: 423982313 [](ancestors = 423982313) |
||
|
|
||
| // Get name of the UIDispatcher property for the IReactPropertyBag. | ||
| static IReactPropertyName UIDispatcherProperty(); | ||
| } | ||
| } // namespace ReactNative | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,6 +265,7 @@ | |
| </ClCompile> | ||
| <ClCompile Include="ABICxxModule.cpp" /> | ||
| <ClCompile Include="ABIViewManager.cpp" /> | ||
| <ClCompile Include="IReactDispatcher.cpp" /> | ||
| <ClCompile Include="Modules\AppStateData.cpp"> | ||
| <Filter>Modules</Filter> | ||
| </ClCompile> | ||
|
|
@@ -298,7 +299,6 @@ | |
| <ClCompile Include="ReactHost\UwpReactInstanceProxy.cpp"> | ||
| <Filter>ReactHost</Filter> | ||
| </ClCompile> | ||
| <ClCompile Include="ReactSupport.cpp" /> | ||
| <ClCompile Include="RedBox.cpp" /> | ||
| <ClCompile Include="TestHook.cpp" /> | ||
|
|
@@ -599,6 +599,7 @@ | |
| <Filter>Base</Filter> | ||
| </ClInclude> | ||
| <ClInclude Include="HResult.h" /> | ||
| <ClInclude Include="IReactDispatcher.h" /> | ||
| <ClInclude Include="LifecycleState.h" /> | ||
| <ClInclude Include="Modules\AppStateData.h"> | ||
| <Filter>Modules</Filter> | ||
|
|
@@ -648,7 +649,6 @@ | |
| <ClInclude Include="ReactHost\UwpReactInstanceProxy.h"> | ||
| <Filter>ReactHost</Filter> | ||
| </ClInclude> | ||
|
Contributor
There was a problem hiding this comment. Why is this removed? #Resolved
Member
Author
There was a problem hiding this comment. 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) |
||
| <ClInclude Include="ReactSupport.h" /> | ||
| <ClInclude Include="RedBox.h" /> | ||
| <ClInclude Include="TestHook.h" /> | ||
|
|
@@ -685,6 +685,7 @@ | |
| <Midl Include="IJSValueWriter.idl" /> | ||
| <Midl Include="ILifecycleEventListener.idl" /> | ||
| <Midl Include="IReactContext.idl" /> | ||
| <Midl Include="IReactDispatcher.idl" /> | ||
| <Midl Include="IReactModuleBuilder.idl" /> | ||
| <Midl Include="IReactPackageBuilder.idl" /> | ||
| <Midl Include="IReactPackageProvider.idl" /> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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::? #ResolvedThere 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)