Skip to content

Commit f727803

Browse files
author
Brian Vaughn
authored
Flush all passive destroy fns before calling create fns (#17947)
* Flush all passive destroy fns before calling create fns Previously we only flushed destroy functions for a single fiber. The reason this is important is that interleaving destroy/create effects between sibling components might cause components to interfere with each other (e.g. a destroy function in one component may unintentionally override a ref value set by a create function in another component). This PR builds on top of the recently added deferPassiveEffectCleanupDuringUnmount kill switch to separate passive effects flushing into two separate phases (similar to layout effects). * Change passive effect flushing to use arrays instead of lists This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored.
1 parent 529e58a commit f727803

File tree

4 files changed

+221
-48
lines changed

4 files changed

+221
-48
lines changed

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

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ import {
110110
captureCommitPhaseError,
111111
resolveRetryThenable,
112112
markCommitTimeOfFallback,
113-
enqueuePendingPassiveEffectDestroyFn,
113+
enqueuePendingPassiveHookEffectMount,
114+
enqueuePendingPassiveHookEffectUnmount,
114115
} from './ReactFiberWorkLoop';
115116
import {
116117
NoEffect as NoHookEffect,
@@ -396,6 +397,28 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
396397
}
397398
}
398399

400+
function schedulePassiveEffects(finishedWork: Fiber) {
401+
if (deferPassiveEffectCleanupDuringUnmount) {
402+
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
403+
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
404+
if (lastEffect !== null) {
405+
const firstEffect = lastEffect.next;
406+
let effect = firstEffect;
407+
do {
408+
const {next, tag} = effect;
409+
if (
410+
(tag & HookPassive) !== NoHookEffect &&
411+
(tag & HookHasEffect) !== NoHookEffect
412+
) {
413+
enqueuePendingPassiveHookEffectUnmount(finishedWork, effect);
414+
enqueuePendingPassiveHookEffectMount(finishedWork, effect);
415+
}
416+
effect = next;
417+
} while (effect !== firstEffect);
418+
}
419+
}
420+
}
421+
399422
export function commitPassiveHookEffects(finishedWork: Fiber): void {
400423
if ((finishedWork.effectTag & Passive) !== NoEffect) {
401424
switch (finishedWork.tag) {
@@ -432,6 +455,10 @@ function commitLifeCycles(
432455
// e.g. a destroy function in one component should never override a ref set
433456
// by a create function in another component during the same commit.
434457
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
458+
459+
if (deferPassiveEffectCleanupDuringUnmount) {
460+
schedulePassiveEffects(finishedWork);
461+
}
435462
return;
436463
}
437464
case ClassComponent: {
@@ -774,7 +801,7 @@ function commitUnmount(
774801
const {destroy, tag} = effect;
775802
if (destroy !== undefined) {
776803
if ((tag & HookPassive) !== NoHookEffect) {
777-
enqueuePendingPassiveEffectDestroyFn(destroy);
804+
enqueuePendingPassiveHookEffectUnmount(current, effect);
778805
} else {
779806
safelyCallDestroy(current, destroy);
780807
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ export type Hook = {|
144144
next: Hook | null,
145145
|};
146146

147-
type Effect = {|
147+
export type Effect = {|
148148
tag: HookEffectTag,
149149
create: () => (() => void) | void,
150150
destroy: (() => void) | void,

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

Lines changed: 112 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
1414
import type {Interaction} from 'scheduler/src/Tracing';
1515
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
1616
import type {SuspenseState} from './ReactFiberSuspenseComponent';
17+
import type {Effect as HookEffect} from './ReactFiberHooks';
1718

1819
import {
1920
warnAboutDeprecatedLifecycles,
@@ -257,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
257258
let rootWithPendingPassiveEffects: FiberRoot | null = null;
258259
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
259260
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
260-
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
261+
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
262+
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
261263

262264
let rootsWithPendingDiscreteUpdates: Map<
263265
FiberRoot,
@@ -2168,11 +2170,28 @@ export function flushPassiveEffects() {
21682170
}
21692171
}
21702172

2171-
export function enqueuePendingPassiveEffectDestroyFn(
2172-
destroy: () => void,
2173+
export function enqueuePendingPassiveHookEffectMount(
2174+
fiber: Fiber,
2175+
effect: HookEffect,
2176+
): void {
2177+
if (deferPassiveEffectCleanupDuringUnmount) {
2178+
pendingPassiveHookEffectsMount.push(effect, fiber);
2179+
if (!rootDoesHavePassiveEffects) {
2180+
rootDoesHavePassiveEffects = true;
2181+
scheduleCallback(NormalPriority, () => {
2182+
flushPassiveEffects();
2183+
return null;
2184+
});
2185+
}
2186+
}
2187+
}
2188+
2189+
export function enqueuePendingPassiveHookEffectUnmount(
2190+
fiber: Fiber,
2191+
effect: HookEffect,
21732192
): void {
21742193
if (deferPassiveEffectCleanupDuringUnmount) {
2175-
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
2194+
pendingPassiveHookEffectsUnmount.push(effect, fiber);
21762195
if (!rootDoesHavePassiveEffects) {
21772196
rootDoesHavePassiveEffects = true;
21782197
scheduleCallback(NormalPriority, () => {
@@ -2183,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
21832202
}
21842203
}
21852204

2205+
function invokePassiveEffectCreate(effect: HookEffect): void {
2206+
const create = effect.create;
2207+
effect.destroy = create();
2208+
}
2209+
21862210
function flushPassiveEffectsImpl() {
21872211
if (rootWithPendingPassiveEffects === null) {
21882212
return false;
@@ -2201,45 +2225,95 @@ function flushPassiveEffectsImpl() {
22012225
const prevInteractions = pushInteractions(root);
22022226

22032227
if (deferPassiveEffectCleanupDuringUnmount) {
2204-
// Flush any pending passive effect destroy functions that belong to
2205-
// components that were unmounted during the most recent commit.
2206-
for (
2207-
let i = 0;
2208-
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
2209-
i++
2210-
) {
2211-
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
2212-
invokeGuardedCallback(null, destroy, null);
2228+
// It's important that ALL pending passive effect destroy functions are called
2229+
// before ANY passive effect create functions are called.
2230+
// Otherwise effects in sibling components might interfere with each other.
2231+
// e.g. a destroy function in one component may unintentionally override a ref
2232+
// value set by a create function in another component.
2233+
// Layout effects have the same constraint.
2234+
2235+
// First pass: Destroy stale passive effects.
2236+
let unmountEffects = pendingPassiveHookEffectsUnmount;
2237+
pendingPassiveHookEffectsUnmount = [];
2238+
for (let i = 0; i < unmountEffects.length; i += 2) {
2239+
const effect = ((unmountEffects[i]: any): HookEffect);
2240+
const fiber = ((unmountEffects[i + 1]: any): Fiber);
2241+
const destroy = effect.destroy;
2242+
effect.destroy = undefined;
2243+
if (typeof destroy === 'function') {
2244+
if (__DEV__) {
2245+
setCurrentDebugFiberInDEV(fiber);
2246+
invokeGuardedCallback(null, destroy, null);
2247+
if (hasCaughtError()) {
2248+
invariant(fiber !== null, 'Should be working on an effect.');
2249+
const error = clearCaughtError();
2250+
captureCommitPhaseError(fiber, error);
2251+
}
2252+
resetCurrentDebugFiberInDEV();
2253+
} else {
2254+
try {
2255+
destroy();
2256+
} catch (error) {
2257+
invariant(fiber !== null, 'Should be working on an effect.');
2258+
captureCommitPhaseError(fiber, error);
2259+
}
2260+
}
2261+
}
22132262
}
2214-
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2215-
}
22162263

2217-
// Note: This currently assumes there are no passive effects on the root
2218-
// fiber, because the root is not part of its own effect list. This could
2219-
// change in the future.
2220-
let effect = root.current.firstEffect;
2221-
while (effect !== null) {
2222-
if (__DEV__) {
2223-
setCurrentDebugFiberInDEV(effect);
2224-
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2225-
if (hasCaughtError()) {
2226-
invariant(effect !== null, 'Should be working on an effect.');
2227-
const error = clearCaughtError();
2228-
captureCommitPhaseError(effect, error);
2264+
// Second pass: Create new passive effects.
2265+
let mountEffects = pendingPassiveHookEffectsMount;
2266+
pendingPassiveHookEffectsMount = [];
2267+
for (let i = 0; i < mountEffects.length; i += 2) {
2268+
const effect = ((mountEffects[i]: any): HookEffect);
2269+
const fiber = ((mountEffects[i + 1]: any): Fiber);
2270+
if (__DEV__) {
2271+
setCurrentDebugFiberInDEV(fiber);
2272+
invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
2273+
if (hasCaughtError()) {
2274+
invariant(fiber !== null, 'Should be working on an effect.');
2275+
const error = clearCaughtError();
2276+
captureCommitPhaseError(fiber, error);
2277+
}
2278+
resetCurrentDebugFiberInDEV();
2279+
} else {
2280+
try {
2281+
const create = effect.create;
2282+
effect.destroy = create();
2283+
} catch (error) {
2284+
invariant(fiber !== null, 'Should be working on an effect.');
2285+
captureCommitPhaseError(fiber, error);
2286+
}
22292287
}
2230-
resetCurrentDebugFiberInDEV();
2231-
} else {
2232-
try {
2233-
commitPassiveHookEffects(effect);
2234-
} catch (error) {
2235-
invariant(effect !== null, 'Should be working on an effect.');
2236-
captureCommitPhaseError(effect, error);
2288+
}
2289+
} else {
2290+
// Note: This currently assumes there are no passive effects on the root fiber
2291+
// because the root is not part of its own effect list.
2292+
// This could change in the future.
2293+
let effect = root.current.firstEffect;
2294+
while (effect !== null) {
2295+
if (__DEV__) {
2296+
setCurrentDebugFiberInDEV(effect);
2297+
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2298+
if (hasCaughtError()) {
2299+
invariant(effect !== null, 'Should be working on an effect.');
2300+
const error = clearCaughtError();
2301+
captureCommitPhaseError(effect, error);
2302+
}
2303+
resetCurrentDebugFiberInDEV();
2304+
} else {
2305+
try {
2306+
commitPassiveHookEffects(effect);
2307+
} catch (error) {
2308+
invariant(effect !== null, 'Should be working on an effect.');
2309+
captureCommitPhaseError(effect, error);
2310+
}
22372311
}
2312+
const nextNextEffect = effect.nextEffect;
2313+
// Remove nextEffect pointer to assist GC
2314+
effect.nextEffect = null;
2315+
effect = nextNextEffect;
22382316
}
2239-
const nextNextEffect = effect.nextEffect;
2240-
// Remove nextEffect pointer to assist GC
2241-
effect.nextEffect = null;
2242-
effect = nextNextEffect;
22432317
}
22442318

22452319
if (enableSchedulerTracing) {

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js‎

Lines changed: 79 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => {
15581558
]);
15591559
});
15601560

1561+
it('unmounts all previous effects between siblings before creating any new ones', () => {
1562+
function Counter({count, label}) {
1563+
useEffect(() => {
1564+
Scheduler.unstable_yieldValue(`Mount ${label} [${count}]`);
1565+
return () => {
1566+
Scheduler.unstable_yieldValue(`Unmount ${label} [${count}]`);
1567+
};
1568+
});
1569+
return <Text text={`${label} ${count}`} />;
1570+
}
1571+
act(() => {
1572+
ReactNoop.render(
1573+
<React.Fragment>
1574+
<Counter label="A" count={0} />
1575+
<Counter label="B" count={0} />
1576+
</React.Fragment>,
1577+
() => Scheduler.unstable_yieldValue('Sync effect'),
1578+
);
1579+
expect(Scheduler).toFlushAndYieldThrough(['A 0', 'B 0', 'Sync effect']);
1580+
expect(ReactNoop.getChildren()).toEqual([span('A 0'), span('B 0')]);
1581+
});
1582+
1583+
expect(Scheduler).toHaveYielded(['Mount A [0]', 'Mount B [0]']);
1584+
1585+
act(() => {
1586+
ReactNoop.render(
1587+
<React.Fragment>
1588+
<Counter label="A" count={1} />
1589+
<Counter label="B" count={1} />
1590+
</React.Fragment>,
1591+
() => Scheduler.unstable_yieldValue('Sync effect'),
1592+
);
1593+
expect(Scheduler).toFlushAndYieldThrough(['A 1', 'B 1', 'Sync effect']);
1594+
expect(ReactNoop.getChildren()).toEqual([span('A 1'), span('B 1')]);
1595+
});
1596+
expect(Scheduler).toHaveYielded([
1597+
'Unmount A [0]',
1598+
'Unmount B [0]',
1599+
'Mount A [1]',
1600+
'Mount B [1]',
1601+
]);
1602+
1603+
act(() => {
1604+
ReactNoop.render(
1605+
<React.Fragment>
1606+
<Counter label="B" count={2} />
1607+
<Counter label="C" count={0} />
1608+
</React.Fragment>,
1609+
() => Scheduler.unstable_yieldValue('Sync effect'),
1610+
);
1611+
expect(Scheduler).toFlushAndYieldThrough(['B 2', 'C 0', 'Sync effect']);
1612+
expect(ReactNoop.getChildren()).toEqual([span('B 2'), span('C 0')]);
1613+
});
1614+
expect(Scheduler).toHaveYielded([
1615+
'Unmount A [1]',
1616+
'Unmount B [1]',
1617+
'Mount B [2]',
1618+
'Mount C [0]',
1619+
]);
1620+
});
1621+
15611622
it('handles errors on mount', () => {
15621623
function Counter(props) {
15631624
useEffect(() => {
@@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => {
16561717
return () => {
16571718
Scheduler.unstable_yieldValue('Oops!');
16581719
throw new Error('Oops!');
1659-
// eslint-disable-next-line no-unreachable
1660-
Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`);
16611720
};
16621721
});
16631722
useEffect(() => {
@@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => {
16681727
});
16691728
return <Text text={'Count: ' + props.count} />;
16701729
}
1730+
16711731
act(() => {
16721732
ReactNoop.render(<Counter count={0} />, () =>
16731733
Scheduler.unstable_yieldValue('Sync effect'),
@@ -1679,18 +1739,30 @@ describe('ReactHooksWithNoopRenderer', () => {
16791739
});
16801740

16811741
act(() => {
1682-
// This update will trigger an error
1742+
// This update will trigger an error during passive effect unmount
16831743
ReactNoop.render(<Counter count={1} />, () =>
16841744
Scheduler.unstable_yieldValue('Sync effect'),
16851745
);
16861746
expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
16871747
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
16881748
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
1689-
expect(Scheduler).toHaveYielded(['Oops!']);
1749+
1750+
// This tests enables a feature flag that flushes all passive destroys in a
1751+
// separate pass before flushing any passive creates.
1752+
// A result of this two-pass flush is that an error thrown from unmount does
1753+
// not block the subsequent create functions from being run.
1754+
expect(Scheduler).toHaveYielded([
1755+
'Oops!',
1756+
'Unmount B [0]',
1757+
'Mount A [1]',
1758+
'Mount B [1]',
1759+
]);
16901760
});
1691-
// B unmounts even though an error was thrown in the previous effect
1692-
// B's destroy function runs later on unmount though, since it's passive
1693-
expect(Scheduler).toHaveYielded(['Unmount B [0]']);
1761+
1762+
// <Counter> gets unmounted because an error is thrown above.
1763+
// The remaining destroy functions are run later on unmount, since they're passive.
1764+
// In this case, one of them throws again (because of how the test is written).
1765+
expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']);
16941766
expect(ReactNoop.getChildren()).toEqual([]);
16951767
});
16961768

0 commit comments

Comments
 (0)