Skip to content

Commit 6ca37ef

Browse files
acdliteacdlite
committed
Check for store mutations before commit
When a store is read for the first time, or when `subscribe` or `getSnapshot` changes, during a concurrent render, we have to check at the end of the render phase whether the store was mutated by an concurrent event. In the userspace shim, we perform this check in a layout effect, and patch up any inconsistencies by scheduling another render + commit. However, even though we patch them up in the next render, the parent layout effects that fire in the original render will still observe an inconsistent tree. In the native implementation, we can instead check for inconsistencies right after the root is completed, before entering the commit phase. If we do detect a mutaiton, we can discard the tree and re-render before firing any effects. The re-render is synchronous to block further concurrent mutations (which is also what we do to recover from tearing bugs that result in an error). After the synchronous re-render, we can assume the tree the tree is consistent and continue with the normal algorithm for finishing a completed root (i.e. either suspend or commit). The result is that layout effects will always observe a consistent tree.
1 parent 5265775 commit 6ca37ef

File tree

7 files changed

+573
-110
lines changed

7 files changed

+573
-110
lines changed

‎packages/react-reconciler/src/ReactFiberFlags.js‎

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -12,51 +12,53 @@ import {enableCreateEventHandleAPI} from 'shared/ReactFeatureFlags';
1212
export type Flags = number;
1313

1414
// Don't change these two values. They're used by React Dev Tools.
15-
export const NoFlags = /* */ 0b00000000000000000000000;
16-
export const PerformedWork = /* */ 0b00000000000000000000001;
15+
export const NoFlags = /* */ 0b000000000000000000000000;
16+
export const PerformedWork = /* */ 0b000000000000000000000001;
1717

1818
// You can change the rest (and add more).
19-
export const Placement = /* */ 0b00000000000000000000010;
20-
export const Update = /* */ 0b00000000000000000000100;
19+
export const Placement = /* */ 0b000000000000000000000010;
20+
export const Update = /* */ 0b000000000000000000000100;
2121
export const PlacementAndUpdate = /* */ Placement | Update;
22-
export const Deletion = /* */ 0b00000000000000000001000;
23-
export const ChildDeletion = /* */ 0b00000000000000000010000;
24-
export const ContentReset = /* */ 0b00000000000000000100000;
25-
export const Callback = /* */ 0b00000000000000001000000;
26-
export const DidCapture = /* */ 0b00000000000000010000000;
27-
export const Ref = /* */ 0b00000000000000100000000;
28-
export const Snapshot = /* */ 0b00000000000001000000000;
29-
export const Passive = /* */ 0b00000000000010000000000;
30-
export const Hydrating = /* */ 0b00000000000100000000000;
22+
export const Deletion = /* */ 0b000000000000000000001000;
23+
export const ChildDeletion = /* */ 0b000000000000000000010000;
24+
export const ContentReset = /* */ 0b000000000000000000100000;
25+
export const Callback = /* */ 0b000000000000000001000000;
26+
export const DidCapture = /* */ 0b000000000000000010000000;
27+
export const Ref = /* */ 0b000000000000000100000000;
28+
export const Snapshot = /* */ 0b000000000000001000000000;
29+
export const Passive = /* */ 0b000000000000010000000000;
30+
export const Hydrating = /* */ 0b000000000000100000000000;
3131
export const HydratingAndUpdate = /* */ Hydrating | Update;
32-
export const Visibility = /* */ 0b00000000001000000000000;
32+
export const Visibility = /* */ 0b000000000001000000000000;
33+
export const StoreConsistency = /* */ 0b000000000010000000000000;
3334

34-
export const LifecycleEffectMask = Passive | Update | Callback | Ref | Snapshot;
35+
export const LifecycleEffectMask =
36+
Passive | Update | Callback | Ref | Snapshot | StoreConsistency;
3537

3638
// Union of all commit flags (flags with the lifetime of a particular commit)
37-
export const HostEffectMask = /* */ 0b00000000001111111111111;
39+
export const HostEffectMask = /* */ 0b000000000011111111111111;
3840

3941
// These are not really side effects, but we still reuse this field.
40-
export const Incomplete = /* */ 0b00000000010000000000000;
41-
export const ShouldCapture = /* */ 0b00000000100000000000000;
42-
export const ForceUpdateForLegacySuspense = /* */ 0b00000001000000000000000;
43-
export const DidPropagateContext = /* */ 0b00000010000000000000000;
44-
export const NeedsPropagation = /* */ 0b00000100000000000000000;
42+
export const Incomplete = /* */ 0b000000000100000000000000;
43+
export const ShouldCapture = /* */ 0b000000001000000000000000;
44+
export const ForceUpdateForLegacySuspense = /* */ 0b000000010000000000000000;
45+
export const DidPropagateContext = /* */ 0b000000100000000000000000;
46+
export const NeedsPropagation = /* */ 0b000001000000000000000000;
4547

4648
// Static tags describe aspects of a fiber that are not specific to a render,
4749
// e.g. a fiber uses a passive effect (even if there are no updates on this particular render).
4850
// This enables us to defer more work in the unmount case,
4951
// since we can defer traversing the tree during layout to look for Passive effects,
5052
// and instead rely on the static flag as a signal that there may be cleanup work.
51-
export const RefStatic = /* */ 0b00001000000000000000000;
52-
export const LayoutStatic = /* */ 0b00010000000000000000000;
53-
export const PassiveStatic = /* */ 0b00100000000000000000000;
53+
export const RefStatic = /* */ 0b000010000000000000000000;
54+
export const LayoutStatic = /* */ 0b000100000000000000000000;
55+
export const PassiveStatic = /* */ 0b001000000000000000000000;
5456

5557
// These flags allow us to traverse to fibers that have effects on mount
5658
// without traversing the entire tree after every commit for
5759
// double invoking
58-
export const MountLayoutDev = /* */ 0b01000000000000000000000;
59-
export const MountPassiveDev = /* */ 0b10000000000000000000000;
60+
export const MountLayoutDev = /* */ 0b010000000000000000000000;
61+
export const MountPassiveDev = /* */ 0b100000000000000000000000;
6062

6163
// Groups of flags that are used in the commit phase to skip over trees that
6264
// don't contain effects, by checking subtreeFlags.

‎packages/react-reconciler/src/ReactFiberHooks.new.js‎

Lines changed: 67 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import {
4444
SyncLane,
4545
NoLanes,
4646
isSubsetOfLanes,
47+
includesBlockingLane,
4748
mergeLanes,
4849
removeLanes,
4950
intersectLanes,
@@ -68,6 +69,7 @@ import {
6869
PassiveStatic as PassiveStaticEffect,
6970
StaticMask as StaticMaskEffect,
7071
Update as UpdateEffect,
72+
StoreConsistency,
7173
} from './ReactFiberFlags';
7274
import {
7375
HasEffect as HookHasEffect,
@@ -166,7 +168,15 @@ type StoreInstance<T> = {|
166168
getSnapshot: () => T,
167169
|};
168170

169-
export type FunctionComponentUpdateQueue = {|lastEffect: Effect | null|};
171+
type StoreConsistencyCheck<T> = {|
172+
value: T,
173+
getSnapshot: () => T,
174+
|};
175+
176+
export type FunctionComponentUpdateQueue = {|
177+
lastEffect: Effect | null,
178+
stores: Array<StoreConsistencyCheck<any>> | null,
179+
|};
170180

171181
type BasicStateAction<S> = (S => S) | S;
172182

@@ -689,6 +699,7 @@ function updateWorkInProgressHook(): Hook {
689699
function createFunctionComponentUpdateQueue(): FunctionComponentUpdateQueue {
690700
return {
691701
lastEffect: null,
702+
stores: null,
692703
};
693704
}
694705

@@ -1289,17 +1300,25 @@ function mountSyncExternalStore<T>(
12891300
// don't need to set a static flag, either.
12901301
// TODO: We can move this to the passive phase once we add a pre-commit
12911302
// consistency check. See the next comment.
1292-
fiber.flags |= UpdateEffect;
1303+
fiber.flags |= PassiveEffect;
12931304
pushEffect(
1294-
HookHasEffect | HookLayout,
1305+
HookHasEffect | HookPassive,
12951306
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
12961307
undefined,
12971308
null,
12981309
);
1299-
// TODO: Unless this is a synchronous render, schedule a consistency check.
1300-
// Right before committing, we will walk the tree and check if any of the
1301-
// stores were mutated.
1302-
// pushConsistencyCheck(inst, getSnapshot, nextSnapshot);
1310+
1311+
// Unless we're rendering a blocking lane, schedule a consistency check. Right
1312+
// before committing, we will walk the tree and check if any of the stores
1313+
// were mutated.
1314+
const root: FiberRoot | null = getWorkInProgressRoot();
1315+
invariant(
1316+
root !== null,
1317+
'Expected a work-in-progress root. This is a bug in React. Please file an issue.',
1318+
);
1319+
if (!includesBlockingLane(root, renderLanes)) {
1320+
pushStoreConsistencyCheck(fiber, getSnapshot, nextSnapshot);
1321+
}
13031322

13041323
return nextSnapshot;
13051324
}
@@ -1348,36 +1367,69 @@ function updateSyncExternalStore<T>(
13481367
(workInProgressHook !== null &&
13491368
workInProgressHook.memoizedState.tag & HookHasEffect)
13501369
) {
1351-
// TODO: We can move this to the passive phase once we add a pre-commit
1352-
// consistency check. See the next comment.
1353-
fiber.flags |= UpdateEffect;
1370+
fiber.flags |= PassiveEffect;
13541371
pushEffect(
1355-
HookHasEffect | HookLayout,
1372+
HookHasEffect | HookPassive,
13561373
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
13571374
undefined,
13581375
null,
13591376
);
13601377

1361-
// TODO: Unless this is a synchronous render, schedule a consistency check.
1378+
// Unless we're rendering a blocking lane, schedule a consistency check.
13621379
// Right before committing, we will walk the tree and check if any of the
13631380
// stores were mutated.
1364-
// pushConsistencyCheck(inst, getSnapshot, nextSnapshot);
1381+
const root: FiberRoot | null = getWorkInProgressRoot();
1382+
invariant(
1383+
root !== null,
1384+
'Expected a work-in-progress root. This is a bug in React. Please file an issue.',
1385+
);
1386+
if (!includesBlockingLane(root, renderLanes)) {
1387+
pushStoreConsistencyCheck(fiber, getSnapshot, nextSnapshot);
1388+
}
13651389
}
13661390

13671391
return nextSnapshot;
13681392
}
13691393

1394+
function pushStoreConsistencyCheck<T>(
1395+
fiber: Fiber,
1396+
getSnapshot: () => T,
1397+
renderedSnapshot: T,
1398+
) {
1399+
fiber.flags |= StoreConsistency;
1400+
const check: StoreConsistencyCheck<T> = {
1401+
getSnapshot,
1402+
value: renderedSnapshot,
1403+
};
1404+
let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any);
1405+
if (componentUpdateQueue === null) {
1406+
componentUpdateQueue = createFunctionComponentUpdateQueue();
1407+
currentlyRenderingFiber.updateQueue = (componentUpdateQueue: any);
1408+
componentUpdateQueue.stores = [check];
1409+
} else {
1410+
const stores = componentUpdateQueue.stores;
1411+
if (stores === null) {
1412+
componentUpdateQueue.stores = [check];
1413+
} else {
1414+
stores.push(check);
1415+
}
1416+
}
1417+
}
1418+
13701419
function updateStoreInstance<T>(
13711420
fiber: Fiber,
13721421
inst: StoreInstance<T>,
13731422
nextSnapshot: T,
13741423
getSnapshot: () => T,
13751424
) {
1425+
// These are updated in the passive phase
13761426
inst.value = nextSnapshot;
13771427
inst.getSnapshot = getSnapshot;
13781428

1379-
// TODO: Move the tearing checks to an earlier, pre-commit phase so that the
1380-
// layout effects always observe a consistent tree.
1429+
// Something may have been mutated in between render and commit. This could
1430+
// have been in an event that fired before the passive effects, or it could
1431+
// have been in a layout effect. In that case, we would have used the old
1432+
// snapsho and getSnapshot values to bail out. We need to check one more time.
13811433
if (checkIfSnapshotChanged(inst)) {
13821434
// Force a re-render.
13831435
forceStoreRerender(fiber);
@@ -1393,11 +1445,6 @@ function subscribeToStore(fiber, inst, subscribe) {
13931445
forceStoreRerender(fiber);
13941446
}
13951447
};
1396-
// Check for changes right before subscribing. Subsequent changes will be
1397-
// detected in the subscription handler.
1398-
// TODO: Once updateStoreInstance is moved to the passive phase, we can rely
1399-
// on that check instead of checking again here.
1400-
handleStoreChange();
14011448
// Subscribe to the store and return a clean-up function.
14021449
return subscribe(handleStoreChange);
14031450
}

0 commit comments

Comments
 (0)