Split mutable source pending expiration times into first and last

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberHooks.js

@@ -75,7 +75,8 @@ import {
   getCurrentPriorityLevel,
 } from './SchedulerWithReactIntegration';
 import {
-  getPendingExpirationTime,
+  getFirstPendingExpirationTime,
+  getLastPendingExpirationTime,
   getWorkInProgressVersion,
   markSourceAsDirty,
   setPendingExpirationTime,
@@ -916,16 +917,14 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
     isSafeToReadFromSource = currentRenderVersion === version;
   } else {
     // If there's no version, then we should fallback to checking the update time.
-    const pendingExpirationTime = getPendingExpirationTime(root);
+    const pendingExpirationTime = getFirstPendingExpirationTime(root);
 
     if (pendingExpirationTime === NoWork) {
       isSafeToReadFromSource = true;
     } else {
       // If the source has pending updates, we can use the current render's expiration
       // time to determine if it's safe to read again from the source.
-      isSafeToReadFromSource =
-        pendingExpirationTime === NoWork ||
-        pendingExpirationTime >= renderExpirationTime;
+      isSafeToReadFromSource = pendingExpirationTime >= renderExpirationTime;
     }
 
     if (isSafeToReadFromSource) {
@@ -974,7 +973,8 @@ function useMutableSource<Source, Snapshot>(
 
   const dispatcher = ReactCurrentDispatcher.current;
 
-  const [currentSnapshot, setSnapshot] = dispatcher.useState(() =>
+  // eslint-disable-next-line prefer-const
+  let [currentSnapshot, setSnapshot] = dispatcher.useState(() =>
     readFromUnsubcribedMutableSource(root, source, getSnapshot),
   );
   let snapshot = currentSnapshot;
@@ -1032,7 +1032,7 @@ function useMutableSource<Source, Snapshot>(
           // There is no mechanism currently to associate these updates though,
           // so for now we fall back to synchronously flushing all pending updates.
           // TODO: Improve this later.
-          markRootExpiredAtTime(root, getPendingExpirationTime(root));
+          markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
         }
       }
     }
@@ -1091,7 +1091,7 @@ function useMutableSource<Source, Snapshot>(
         // We missed a mutation before committing.
         // It's possible that other components using this source also have pending updates scheduled.
         // In that case, we should ensure they all commit together.
-        markRootExpiredAtTime(root, getPendingExpirationTime(root));
+        markRootExpiredAtTime(root, getLastPendingExpirationTime(root));
       }
     }
 

packages/react-reconciler/src/ReactFiberRoot.js

@@ -79,7 +79,8 @@ type BaseFiberRootProperties = {|
   lastExpiredTime: ExpirationTime,
   // Used by useMutableSource hook to avoid tearing within this root
   // when external, mutable sources are read from during render.
-  mutableSourcePendingUpdateTime: ExpirationTime,
+  mutableSourceFirstPendingUpdateTime: ExpirationTime,
+  mutableSourceLastPendingUpdateTime: ExpirationTime,
 |};
 
 // The following attributes are only used by interaction tracing builds.
@@ -130,7 +131,8 @@ function FiberRootNode(containerInfo, tag, hydrate) {
   this.nextKnownPendingLevel = NoWork;
   this.lastPingedTime = NoWork;
   this.lastExpiredTime = NoWork;
-  this.mutableSourcePendingUpdateTime = NoWork;
+  this.mutableSourceFirstPendingUpdateTime = NoWork;
+  this.mutableSourceLastPendingUpdateTime = NoWork;
 
   if (enableSchedulerTracing) {
     this.interactionThreadID = unstable_getThreadID();

packages/react-reconciler/src/ReactMutableSource.js

@@ -30,13 +30,20 @@ export function clearPendingUpdates(
   root: FiberRoot,
   expirationTime: ExpirationTime,
 ): void {
-  if (root.mutableSourcePendingUpdateTime <= expirationTime) {
-    root.mutableSourcePendingUpdateTime = NoWork;
+  if (root.mutableSourceFirstPendingUpdateTime <= expirationTime) {
+    root.mutableSourceFirstPendingUpdateTime = NoWork;
   }
+  if (root.mutableSourceLastPendingUpdateTime <= expirationTime) {
+    root.mutableSourceLastPendingUpdateTime = NoWork;
+  }
+}
+
+export function getFirstPendingExpirationTime(root: FiberRoot): ExpirationTime {
+  return root.mutableSourceFirstPendingUpdateTime;
 }
 
-export function getPendingExpirationTime(root: FiberRoot): ExpirationTime {
-  return root.mutableSourcePendingUpdateTime;
+export function getLastPendingExpirationTime(root: FiberRoot): ExpirationTime {
+  return root.mutableSourceLastPendingUpdateTime;
 }
 
 export function setPendingExpirationTime(
@@ -46,13 +53,19 @@ export function setPendingExpirationTime(
   // Because we only track one pending update time per root,
   // track the lowest priority update.
   // It's inclusive of all other pending updates.
-  //
-  // TODO This currently gives us a false positive in some cases
-  // when it comes to determining if it's safe to read during an update.
-  root.mutableSourcePendingUpdateTime = Math.max(
-    root.mutableSourcePendingUpdateTime,
+  root.mutableSourceLastPendingUpdateTime = Math.max(
+    root.mutableSourceLastPendingUpdateTime,
     expirationTime,
   );
+
+  if (root.mutableSourceFirstPendingUpdateTime === NoWork) {
+    root.mutableSourceFirstPendingUpdateTime = expirationTime;
+  } else {
+    root.mutableSourceFirstPendingUpdateTime = Math.min(
+      root.mutableSourceFirstPendingUpdateTime,
+      expirationTime,
+    );
+  }
 }
 
 export function markSourceAsDirty(mutableSource: MutableSource<any>): void {

packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js

@@ -1237,7 +1237,7 @@ describe('useMutableSource', () => {
         ReactNoop.discreteUpdates(() => {
           // At high priority, toggle y so that it reads from A instead of B.
           // Simultaneously, mutate A.
-          mutateA('high-pri baz');
+          mutateA('baz');
           root.render(
             <App
               getSnapshotFirst={getSnapshotA}
@@ -1246,7 +1246,7 @@ describe('useMutableSource', () => {
           );
 
           // If this update were processed before the next mutation,
-          // it would be expected to yield "high-pri baz" and "high-pri baz".
+          // it would be expected to yield "baz" and "baz".
         });
 
         // At lower priority, mutate A again.