Wrap retrySuspendedRoot using SchedulerTracing (#13776)

sophiebits · web-flow · commit d83601080a7c · 2018-10-04T15:11:12.000-07:00
Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.)

The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -43,14 +43,12 @@ import {
   Update,
   Ref,
 } from 'shared/ReactSideEffectTags';
-import {captureWillSyncRenderPlaceholder} from './ReactFiberScheduler';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import {
   enableSuspense,
   debugRenderPhaseSideEffects,
   debugRenderPhaseSideEffectsForStrictMode,
   enableProfilerTimer,
-  enableSchedulerTracing,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import shallowEqual from 'shared/shallowEqual';
@@ -952,13 +950,6 @@ function updatePlaceholderComponent(
 
     let nextDidTimeout;
     if (current !== null && workInProgress.updateQueue !== null) {
-      if (enableSchedulerTracing) {
-        // Handle special case of rendering a Placeholder for a sync, suspended tree.
-        // We flag this to properly trace and count interactions.
-        // Otherwise interaction pending count will be decremented too many times.
-        captureWillSyncRenderPlaceholder();
-      }
-
       // We're outside strict mode. Something inside this Placeholder boundary
       // suspended during the last commit. Switch to the placholder.
       workInProgress.updateQueue = null;
diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -257,11 +257,6 @@ let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
 // Used for performance tracking.
 let interruptedBy: Fiber | null = null;
 
-// Do not decrement interaction counts in the event of suspense timeouts.
-// This would lead to prematurely calling the interaction-complete hook.
-// Instead we wait until the suspended promise has resolved.
-let nextRenderIncludesTimedOutPlaceholder: boolean = false;
-
 let stashedWorkInProgressProperties;
 let replayUnitOfWork;
 let isReplayingFailedUnitOfWork;
@@ -766,42 +761,40 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
         unhandledError = error;
       }
     } finally {
-      if (!nextRenderIncludesTimedOutPlaceholder) {
-        // Clear completed interactions from the pending Map.
-        // Unless the render was suspended or cascading work was scheduled,
-        // In which case– leave pending interactions until the subsequent render.
-        const pendingInteractionMap = root.pendingInteractionMap;
-        pendingInteractionMap.forEach(
-          (scheduledInteractions, scheduledExpirationTime) => {
-            // Only decrement the pending interaction count if we're done.
-            // If there's still work at the current priority,
-            // That indicates that we are waiting for suspense data.
-            if (
-              earliestRemainingTimeAfterCommit === NoWork ||
-              scheduledExpirationTime < earliestRemainingTimeAfterCommit
-            ) {
-              pendingInteractionMap.delete(scheduledExpirationTime);
-
-              scheduledInteractions.forEach(interaction => {
-                interaction.__count--;
-
-                if (subscriber !== null && interaction.__count === 0) {
-                  try {
-                    subscriber.onInteractionScheduledWorkCompleted(interaction);
-                  } catch (error) {
-                    // It's not safe for commitRoot() to throw.
-                    // Store the error for now and we'll re-throw in finishRendering().
-                    if (!hasUnhandledError) {
-                      hasUnhandledError = true;
-                      unhandledError = error;
-                    }
+      // Clear completed interactions from the pending Map.
+      // Unless the render was suspended or cascading work was scheduled,
+      // In which case– leave pending interactions until the subsequent render.
+      const pendingInteractionMap = root.pendingInteractionMap;
+      pendingInteractionMap.forEach(
+        (scheduledInteractions, scheduledExpirationTime) => {
+          // Only decrement the pending interaction count if we're done.
+          // If there's still work at the current priority,
+          // That indicates that we are waiting for suspense data.
+          if (
+            earliestRemainingTimeAfterCommit === NoWork ||
+            scheduledExpirationTime < earliestRemainingTimeAfterCommit
+          ) {
+            pendingInteractionMap.delete(scheduledExpirationTime);
+
+            scheduledInteractions.forEach(interaction => {
+              interaction.__count--;
+
+              if (subscriber !== null && interaction.__count === 0) {
+                try {
+                  subscriber.onInteractionScheduledWorkCompleted(interaction);
+                } catch (error) {
+                  // It's not safe for commitRoot() to throw.
+                  // Store the error for now and we'll re-throw in finishRendering().
+                  if (!hasUnhandledError) {
+                    hasUnhandledError = true;
+                    unhandledError = error;
                   }
                 }
-              });
-            }
-          },
-        );
-      }
+              }
+            });
+          }
+        },
+      );
     }
   }
 }
@@ -1173,11 +1166,6 @@ function renderRoot(
     root.pendingCommitExpirationTime = NoWork;
 
     if (enableSchedulerTracing) {
-      // Reset this flag once we start rendering a new root or at a new priority.
-      // This might indicate that suspended work has completed.
-      // If not, the flag will be reset.
-      nextRenderIncludesTimedOutPlaceholder = false;
-
       // Determine which interactions this batch of work currently includes,
       // So that we can accurately attribute time spent working on it,
       // And so that cascading work triggered during the render phase will be associated with it.
@@ -1388,9 +1376,6 @@ function renderRoot(
 
   if (enableSuspense && !isExpired && nextLatestAbsoluteTimeoutMs !== -1) {
     // The tree was suspended.
-    if (enableSchedulerTracing) {
-      nextRenderIncludesTimedOutPlaceholder = true;
-    }
     const suspendedExpirationTime = expirationTime;
     markSuspendedPriorityLevel(root, suspendedExpirationTime);
 
@@ -1597,6 +1582,13 @@ function retrySuspendedRoot(
       markPendingPriorityLevel(root, retryTime);
     }
 
+    // TODO: If the placeholder fiber has already rendered the primary children
+    // without suspending (that is, all of the promises have already resolved),
+    // we should not trigger another update here. One case this happens is when
+    // we are in sync mode and a single promise is thrown both on initial render
+    // and on update; we attach two .then(retrySuspendedRoot) callbacks and each
+    // one performs Sync work, rerendering the Placeholder.
+
     if ((fiber.mode & ConcurrentMode) !== NoContext) {
       if (root === nextRoot && nextRenderExpirationTime === suspendedTime) {
         // Received a ping at the same priority level at which we're currently
@@ -1614,6 +1606,15 @@ function retrySuspendedRoot(
 }
 
 function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null {
+  recordScheduleUpdate();
+
+  if (__DEV__) {
+    if (fiber.tag === ClassComponent || fiber.tag === ClassComponentLazy) {
+      const instance = fiber.stateNode;
+      warnAboutInvalidUpdates(instance);
+    }
+  }
+
   // Update the source fiber's expiration time
   if (
     fiber.expirationTime === NoWork ||
@@ -1631,57 +1632,47 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null {
   }
   // Walk the parent path to the root and update the child expiration time.
   let node = fiber.return;
+  let root = null;
   if (node === null && fiber.tag === HostRoot) {
-    return fiber.stateNode;
-  }
-  while (node !== null) {
-    alternate = node.alternate;
-    if (
-      node.childExpirationTime === NoWork ||
-      node.childExpirationTime > expirationTime
-    ) {
-      node.childExpirationTime = expirationTime;
+    root = fiber.stateNode;
+  } else {
+    while (node !== null) {
+      alternate = node.alternate;
       if (
+        node.childExpirationTime === NoWork ||
+        node.childExpirationTime > expirationTime
+      ) {
+        node.childExpirationTime = expirationTime;
+        if (
+          alternate !== null &&
+          (alternate.childExpirationTime === NoWork ||
+            alternate.childExpirationTime > expirationTime)
+        ) {
+          alternate.childExpirationTime = expirationTime;
+        }
+      } else if (
         alternate !== null &&
         (alternate.childExpirationTime === NoWork ||
           alternate.childExpirationTime > expirationTime)
       ) {
         alternate.childExpirationTime = expirationTime;
       }
-    } else if (
-      alternate !== null &&
-      (alternate.childExpirationTime === NoWork ||
-        alternate.childExpirationTime > expirationTime)
-    ) {
-      alternate.childExpirationTime = expirationTime;
-    }
-    if (node.return === null && node.tag === HostRoot) {
-      return node.stateNode;
-    }
-    node = node.return;
-  }
-  return null;
-}
-
-function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
-  recordScheduleUpdate();
-
-  if (__DEV__) {
-    if (fiber.tag === ClassComponent || fiber.tag === ClassComponentLazy) {
-      const instance = fiber.stateNode;
-      warnAboutInvalidUpdates(instance);
+      if (node.return === null && node.tag === HostRoot) {
+        root = node.stateNode;
+        break;
+      }
+      node = node.return;
     }
   }
 
-  const root = scheduleWorkToRoot(fiber, expirationTime);
   if (root === null) {
     if (
       __DEV__ &&
       (fiber.tag === ClassComponent || fiber.tag === ClassComponentLazy)
     ) {
       warnAboutUpdateOnUnmounted(fiber);
     }
-    return;
+    return null;
   }
 
   if (enableSchedulerTracing) {
@@ -1718,6 +1709,15 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
     }
   }
 
+  return root;
+}
+
+function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
+  const root = scheduleWorkToRoot(fiber, expirationTime);
+  if (root === null) {
+    return;
+  }
+
   if (
     !isWorking &&
     nextRenderExpirationTime !== NoWork &&
@@ -1904,15 +1904,7 @@ function onTimeout(root, finishedWork, suspendedExpirationTime) {
     // because we're at the top of a timer event.
     recomputeCurrentRendererTime();
     currentSchedulerTime = currentRendererTime;
-
-    if (enableSchedulerTracing) {
-      // Don't update pending interaction counts for suspense timeouts,
-      // Because we know we still need to do more work in this case.
-      nextRenderIncludesTimedOutPlaceholder = true;
-      flushRoot(root, suspendedExpirationTime);
-    } else {
-      flushRoot(root, suspendedExpirationTime);
-    }
+    flushRoot(root, suspendedExpirationTime);
   }
 }
 
@@ -2480,12 +2472,6 @@ function flushControlled(fn: () => mixed): void {
   }
 }
 
-function captureWillSyncRenderPlaceholder() {
-  if (enableSchedulerTracing) {
-    nextRenderIncludesTimedOutPlaceholder = true;
-  }
-}
-
 export {
   requestCurrentTime,
   computeExpirationForFiber,
@@ -2508,5 +2494,4 @@ export {
   interactiveUpdates,
   flushInteractiveUpdates,
   computeUniqueAsyncExpiration,
-  captureWillSyncRenderPlaceholder,
 };
diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js
@@ -14,6 +14,7 @@ import type {CapturedValue} from './ReactCapturedValue';
 import type {Update} from './ReactUpdateQueue';
 import type {Thenable} from './ReactFiberScheduler';
 
+import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
 import getComponentName from 'shared/getComponentName';
 import warningWithoutStack from 'shared/warningWithoutStack';
 import {
@@ -59,7 +60,6 @@ import {
   markLegacyErrorBoundaryAsFailed,
   isAlreadyFailedLegacyErrorBoundary,
   retrySuspendedRoot,
-  captureWillSyncRenderPlaceholder,
 } from './ReactFiberScheduler';
 import {Sync} from './ReactFiberExpirationTime';
 
@@ -224,12 +224,15 @@ function throwException(
               : renderExpirationTime;
 
           // Attach a listener to the promise to "ping" the root and retry.
-          const onResolveOrReject = retrySuspendedRoot.bind(
+          let onResolveOrReject = retrySuspendedRoot.bind(
             null,
             root,
             workInProgress,
             pingTime,
           );
+          if (enableSchedulerTracing) {
+            onResolveOrReject = Schedule_tracing_wrap(onResolveOrReject);
+          }
           thenable.then(onResolveOrReject, onResolveOrReject);
 
           // If the boundary is outside of strict mode, we should *not* suspend
@@ -243,13 +246,6 @@ function throwException(
           if ((workInProgress.mode & StrictMode) === NoEffect) {
             workInProgress.effectTag |= UpdateEffect;
 
-            if (enableSchedulerTracing) {
-              // Handles the special case of unwinding a suspended sync render.
-              // We flag this to properly trace and count interactions.
-              // Otherwise interaction pending count will be decremented too many times.
-              captureWillSyncRenderPlaceholder();
-            }
-
             // Unmount the source fiber's children
             const nextChildren = null;
             reconcileChildren(
diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js
