Add UIDispatcher property to ReactInstanceSettings and IReactContext #5007

vmoroz · 2020-05-25T23:10:27Z

In this PR we simplify access to UIDispatcher, add verification if UIDispatcher is missing, and fixing UIDispatcher and Mso::DispatchQueue issues found by unit tests.

The ReactInstanceSettings and IReactContext interfaces have the new UIDispatcher property. It is a shortcut to the UIDispatcher stored in the Properties property bag accessed with ReactDispatcherHelper::UIDispaptcherProperty.

If the ReactInstanceSettings is initialized from a UI thread, then the UIDispatcher property is automatically initialized. Otherwise, it is null. New unit tests are added to verify it.

The PR is also addressing these issues found while running the new tests:

Make sure that we return the same ReactDispatcher and Mso::DispatchQueue for the same UI thread. To achieve it, we store the UI Mso::DispatchQueue in a special per-thread registry. The UI ReactDispatcher is stored in the Mso::DispatchQueue local queue store.
Fixed the Mso::DispatchQueue local queue store implementation.

Changes in the NativeMyModuleSpec.g.h are unrelated, but they appear every time we run the yarn build.

Microsoft Reviewers: Open in CodeFlow

acoates-ms · 2020-05-26T15:53:04Z

vnext/Microsoft.ReactNative/IReactDispatcher.cpp

+  IReactDispatcher dispatcher;
+  auto queue = Mso::DispatchQueue::GetCurrentUIThreadQueue();
+  if (queue) {
+    queue.InvokeElsePost([&queue, &dispatcher ]() noexcept {


Isn't there an issue if this ends up posting. Wouldn't it be non-deterministic if dispatcher gets set before the end of the function or not?

Should this instead be a check of HasThreadAccess? #Resolved

In this context I have got the queue from the thread. It must have the thread access. I can add the HasThreadAccess to be more obvious and a comment why it is done (to access the Queue Local values).

In reply to: 430521560 [](ancestors = 430521560)

Added the HasThreadAccess check and comments.

In reply to: 430588145 [](ancestors = 430588145,430521560)

NikoAri · 2020-05-26T16:08:32Z

vnext/Microsoft.ReactNative/IReactContext.h

 #include "winrt/Microsoft.ReactNative.h"

-namespace winrt::Microsoft::ReactNative {
+namespace winrt::Microsoft::ReactNative::implementation {


implementation [](start = 41, length = 14)

(nit) is that a common pattern? #Resolved

Yes, the C++/WinRT usually puts the implementation in the winrt::Microsoft::ReactNative::implementation namespace.
Previously here I did not do it, just to make the namespace shorter, but now we have another ReactContext struct in the winrt::Microsoft::ReactNative namespace coming from the MS.RN.Cxx project and we must change the namespace here to avoid the naming conflict.

In reply to: 430532708 [](ancestors = 430532708)

dannyvv · 2020-05-26T17:36:44Z

vnext/Microsoft.ReactNative/IReactDispatcher.idl


    // Get name of the UIDispatcher property for the IReactPropertyBag.
-    static IReactPropertyName UIDispatcherProperty();
+    static IReactPropertyName UIDispatcherProperty { get; };


UIDispatcherProperty [](start = 30, length = 20)

just checking: Is it common in WinRT to post-fix properties? Or should it be UIDispatcher now that we added a getter? #Resolved

Note that this is not a real property that returns UIDispatcher. It is a property that can be used to access UIDispatcher from a property bag. This is why it has the 'Property' suffix.
E.g. properties.Get(ReactDispatcherHelper.UIDispatcherProperty).

In reply to: 430588974 [](ancestors = 430588974)

dannyvv · 2020-05-26T17:39:37Z

vnext/Microsoft.ReactNative/IReactDispatcher.cpp

-/*static*/ ReactPropertyId<IReactDispatcher> ReactDispatcher::UIDispatcherProperty() noexcept {
-  static ReactPropertyId<IReactDispatcher> uiThreadDispatcherProperty{L"ReactNative.Dispatcher", L"UIDispatcher"};
+  static thread_local weak_ref<IReactDispatcher> *tlsWeakDispatcher{nullptr};
+  IReactDispatcher dispatcher;


dispatcher [](start = 19, length = 10)

uninitalized memory if queue is null? Or did I miss a new c++/winrt feature? #Resolved

The interface-like looking types in C++/WinRT are really specialized com_ptr smart pointers. This line calls the default constructor of the IReactDispatcher that sets it to nullptr. I can make it more explicit to avoid any confusion.

In reply to: 430590699 [](ancestors = 430590699)

doh, sorry... thanks!, No need. Still thinking COM, not WinRT :)

In reply to: 430592838 [](ancestors = 430592838,430590699)

Added passing nullptr to constructor to make it explicit.

In reply to: 430593326 [](ancestors = 430593326,430592838,430590699)

dannyvv · 2020-05-26T17:51:46Z

packages/microsoft-reactnative-sampleapps/codegen/NativeMyModuleSpec.g.h

-      SyncMethod<JSValueObject(JSValueObject) noexcept>{5, L"getObject"},
-      SyncMethod<JSValueObject(double, std::string, JSValueObject) noexcept>{6, L"getValue"},
-      Method<void(Callback<JSValue>) noexcept>{7, L"getValueWithCallback"},
+      SyncMethod<React::JSValueArray(React::JSValueArray) noexcept>{4, L"getArray"},


Just for my sanity:
I didn't see any change to something that looks like a code generator, nor a package version update in case the codegen is part of that... Did you manually change this? #Resolved

No, I did not. This file is updated every time I run 'yarn build'. The change is related to the recent codegen changes by Andrew Coates.

In reply to: 430597854 [](ancestors = 430597854)

acoates-ms · 2020-05-26T18:10:46Z

Can we remove the code that sets the UIDispatcher in the playground app. As that shouldn't be needed now that it gets automatically set when the settings are created on a UI thread. (which I'm pretty sure they are in playground) #Resolved

vmoroz · 2020-05-26T19:30:17Z

Yes, I can do it too.

In reply to: 634189457 [](ancestors = 634189457)

vmoroz · 2020-05-26T20:27:20Z

I have removed the UIDispatcher property setting from the Playground projects and they continue to work.

In reply to: 634230910 [](ancestors = 634230910,634189457)

ghost · 2020-05-26T21:08:37Z

Hello @vmoroz!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

NickGerleman · 2020-06-02T03:11:57Z

Moving from "Request Backport" to "Backport Approved" after earlier discussion.

* Implemented support for native module std::weak_ptr (#4980) (cherry picked from commit c23d3de) # Conflicts: # packages/microsoft-reactnative-sampleapps/windows/SampleLibraryCPP/SampleModuleCPP.h * Fix ReactInstance error state to avoid crashes (#4986) * Fix ReactInstance error state to avoid crashes * Change files * Clean m_redboxContent after RedBox closing (cherry picked from commit 05779f2) * ReactNotificationService to allow communication between native modules (#4953) Only picking up changes required for ReactDispatcher. (cherry picked from commit baba475) * Add UIDispatcher property to ReactInstanceSettings and IReactContext (#5007) * Add UIDispatcher property to ReactInstanceSettings and IReactContext * Change files * Fix ReactWindows-Desktop.sln compilation * Address PR feedback for the GetCurrentUIThreadQueue * Remove UIDispatcher setting from Playground project (cherry picked from commit 7236d61) # Conflicts: # packages/playground/windows/playground-win32/Playground-Win32.cpp # vnext/Desktop/React.Windows.Desktop.vcxproj.filters # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactContextTest.cpp # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactModuleBuilderMock.h # vnext/Microsoft.ReactNative.Cxx/ReactContext.h # vnext/Microsoft.ReactNative.Managed.UnitTests/ReactModuleBuilderMock.cs # vnext/Microsoft.ReactNative/IReactContext.cpp # vnext/Microsoft.ReactNative/IReactContext.h # vnext/Microsoft.ReactNative/IReactContext.idl # vnext/Microsoft.ReactNative/ReactInstanceSettings.h # vnext/Microsoft.ReactNative/ReactInstanceSettings.idl * Fix PlaygroundWin32 compilation

vmoroz added 2 commits May 25, 2020 15:55

Add UIDispatcher property to ReactInstanceSettings and IReactContext

eb9d064

Change files

db98d7c

vmoroz requested review from a team, acoates-ms and aeulitz May 25, 2020 23:10

vmoroz requested a review from a team as a code owner May 25, 2020 23:10

acoates-ms reviewed May 26, 2020

View reviewed changes

NikoAri reviewed May 26, 2020

View reviewed changes

Fix ReactWindows-Desktop.sln compilation

22245fc

dannyvv reviewed May 26, 2020

View reviewed changes

Address PR feedback for the GetCurrentUIThreadQueue

40d5e6e

dannyvv approved these changes May 26, 2020

View reviewed changes

Remove UIDispatcher setting from Playground project

122b3ab

acoates-ms approved these changes May 26, 2020

View reviewed changes

vmoroz added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label May 26, 2020

vmoroz added 2 commits May 26, 2020 14:13

Merge branch 'master' into PR/UIDispatcherProperty

fdecd92

Merge branch 'master' into PR/UIDispatcherProperty

f916be0

ghost merged commit 7236d61 into microsoft:master May 26, 2020

Khalef1 added the Request Backport label May 28, 2020

NickGerleman mentioned this pull request Jun 2, 2020

Fix Playground app crash on instance load #4896

Merged

NickGerleman added Backport Approved and removed Request Backport labels Jun 2, 2020

vmoroz mentioned this pull request Jun 5, 2020

Cherry pick API changes for 0.62 #5133

Merged

NickGerleman added Backported to 0.62 and removed Backport Approved labels Jun 10, 2020

This pull request was closed.

Add UIDispatcher property to ReactInstanceSettings and IReactContext #5007

Add UIDispatcher property to ReactInstanceSettings and IReactContext #5007

Uh oh!

Conversation

vmoroz commented May 25, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

acoates-ms May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

NikoAri May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dannyvv May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dannyvv May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dannyvv May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dannyvv May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz May 26, 2020

Choose a reason for hiding this comment

Uh oh!

acoates-ms commented May 26, 2020 • edited by vmoroz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmoroz commented May 26, 2020

Uh oh!

vmoroz commented May 26, 2020

Uh oh!

ghost commented May 26, 2020

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 (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

NickGerleman commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vmoroz commented May 25, 2020 •

edited by ghost

Loading

acoates-ms May 26, 2020 •

edited by vmoroz

Loading

NikoAri May 26, 2020 •

edited by vmoroz

Loading

dannyvv May 26, 2020 •

edited by vmoroz

Loading

dannyvv May 26, 2020 •

edited by vmoroz

Loading

dannyvv May 26, 2020 •

edited

Loading

dannyvv May 26, 2020 •

edited by vmoroz

Loading

acoates-ms commented May 26, 2020 •

edited by vmoroz

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.