From dffec8bc7bee4d05b72d19b4efe4aeab980f4cd5 Mon Sep 17 00:00:00 2001
From: Samuel Susla <samuelsusla@fb.com>
Date: Sun, 16 Aug 2020 03:47:44 -0700
Subject: [PATCH] Use ShadowTree::commit if failureCallback is nullptr

Summary:
Changelog: [internal]

In D22940187 (https://github.com/facebook/react-native/commit/774dec1e17f6f250172a6d4d944121b82fa36efb) we introduced a mechanism to retry failed state updates from view layer.
The mechanism fixes an issue where state update is occasionally dropped with background executor enabled.

# Why is state dropped?

The state is dropped because with background executor enabled, it is possible to enter `ShadowTree::tryCommit` for the same tree from 2 different threads at once. One of thread changes `_rootShadowNode` causing the other commit to fail.

The code goes like this:

```
{
    Lock mutex
    grab reference to `rootShadowNode_` and call it `oldRootShadowNode`
    Unlock mutex
}

State reconciliation
Trigger layout
`rootShadowNode_` is untouched in this section.

{
    Lock mutex
    Check if `oldRootShadowNode` is equal to `rootShadowNode_`, if not, return false signalling failure. Now this is what happens when
    state update fails.

..... not relevant

    Unlock mutex
}

..... not relevant
```

However, in D22940187 (https://github.com/facebook/react-native/commit/774dec1e17f6f250172a6d4d944121b82fa36efb), we have taken another path. Instead of retrying to commit transaction, client is informed about the failure and it is left up to them to retry. This is correct and works. But I think it is unnecessary to this retry can be done inside UIManager::updateState.

In this diff I call `ShadowTree::commit` (version with retries) in case no `stateUpdate.failureCallback` is provided.
This makes sure that we do retry if state update fails but if Android implements `stateUpdate.failureCallback`, it is left up to view layer to retry.

Eventually we might decide to converge these two approaches.

Reviewed By: shergin

Differential Revision: D23151218

fbshipit-source-id: 5cb78a3a75a754429a8e33bd7736e683e9ed34d4
---
 .../react/renderer/uimanager/UIManager.cpp    | 23 +++++++++++++------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/ReactCommon/react/renderer/uimanager/UIManager.cpp b/ReactCommon/react/renderer/uimanager/UIManager.cpp
index 91827e3fb94..e9e48ea856f 100644
--- a/ReactCommon/react/renderer/uimanager/UIManager.cpp
+++ b/ReactCommon/react/renderer/uimanager/UIManager.cpp
@@ -238,10 +238,10 @@ void UIManager::updateState(StateUpdate const &stateUpdate) const {
 
   shadowTreeRegistry_.visit(
       family->getSurfaceId(), [&](ShadowTree const &shadowTree) {
-        bool updateSucceeded = shadowTree.tryCommit(
-            [&](RootShadowNode::Shared const &oldRootShadowNode) {
-              return std::static_pointer_cast<
-                  RootShadowNode>(oldRootShadowNode->cloneTree(
+        auto transaction = [&](RootShadowNode::Shared const &oldRootShadowNode)
+            -> RootShadowNode::Unshared {
+          return std::static_pointer_cast<RootShadowNode>(
+              oldRootShadowNode->cloneTree(
                   *family, [&](ShadowNode const &oldShadowNode) {
                     auto newData =
                         callback(oldShadowNode.getState()->getDataPointer());
@@ -255,9 +255,18 @@ void UIManager::updateState(StateUpdate const &stateUpdate) const {
                         /* .state = */ newState,
                     });
                   }));
-            });
-        if (!updateSucceeded && stateUpdate.failureCallback) {
-          stateUpdate.failureCallback();
+        };
+        if (stateUpdate.failureCallback) {
+          // If caller passes failure callback, we don't retry but let the
+          // caller handle failure.
+          bool updateSucceeded = shadowTree.tryCommit(transaction);
+          if (!updateSucceeded && stateUpdate.failureCallback) {
+            stateUpdate.failureCallback();
+          }
+        } else {
+          // If caller does't provide failure block, commit is attempted until
+          // it succeeds.
+          shadowTree.commit(transaction);
         }
       });
 }