Skip to content

Commit

Permalink
Flush all passive destroy fns before calling create fns (#17947)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
Brian Vaughn authored Feb 11, 2020
1 parent 529e58a commit f727803
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 48 deletions.
31 changes: 29 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
enqueuePendingPassiveEffectDestroyFn,
enqueuePendingPassiveHookEffectMount,
enqueuePendingPassiveHookEffectUnmount,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
Expand Down Expand Up @@ -396,6 +397,28 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
}
}

function schedulePassiveEffects(finishedWork: Fiber) {
if (deferPassiveEffectCleanupDuringUnmount) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
const {next, tag} = effect;
if (
(tag & HookPassive) !== NoHookEffect &&
(tag & HookHasEffect) !== NoHookEffect
) {
enqueuePendingPassiveHookEffectUnmount(finishedWork, effect);
enqueuePendingPassiveHookEffectMount(finishedWork, effect);
}
effect = next;
} while (effect !== firstEffect);
}
}
}

export function commitPassiveHookEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
Expand Down Expand Up @@ -432,6 +455,10 @@ function commitLifeCycles(
// e.g. a destroy function in one component should never override a ref set
// by a create function in another component during the same commit.
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);

if (deferPassiveEffectCleanupDuringUnmount) {
schedulePassiveEffects(finishedWork);
}
return;
}
case ClassComponent: {
Expand Down Expand Up @@ -774,7 +801,7 @@ function commitUnmount(
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookPassive) !== NoHookEffect) {
enqueuePendingPassiveEffectDestroyFn(destroy);
enqueuePendingPassiveHookEffectUnmount(current, effect);
} else {
safelyCallDestroy(current, destroy);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export type Hook = {|
next: Hook | null,
|};

type Effect = {|
export type Effect = {|
tag: HookEffectTag,
create: () => (() => void) | void,
destroy: (() => void) | void,
Expand Down
150 changes: 112 additions & 38 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
import type {Interaction} from 'scheduler/src/Tracing';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {SuspenseState} from './ReactFiberSuspenseComponent';
import type {Effect as HookEffect} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -257,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];

let rootsWithPendingDiscreteUpdates: Map<
FiberRoot,
Expand Down Expand Up @@ -2168,11 +2170,28 @@ export function flushPassiveEffects() {
}
}

export function enqueuePendingPassiveEffectDestroyFn(
destroy: () => void,
export function enqueuePendingPassiveHookEffectMount(
fiber: Fiber,
effect: HookEffect,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingPassiveHookEffectsMount.push(effect, fiber);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
flushPassiveEffects();
return null;
});
}
}
}

export function enqueuePendingPassiveHookEffectUnmount(
fiber: Fiber,
effect: HookEffect,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand All @@ -2183,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
}
}

function invokePassiveEffectCreate(effect: HookEffect): void {
const create = effect.create;
effect.destroy = create();
}

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
Expand All @@ -2201,45 +2225,95 @@ function flushPassiveEffectsImpl() {
const prevInteractions = pushInteractions(root);

if (deferPassiveEffectCleanupDuringUnmount) {
// Flush any pending passive effect destroy functions that belong to
// components that were unmounted during the most recent commit.
for (
let i = 0;
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
i++
) {
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
invokeGuardedCallback(null, destroy, null);
// It's important that ALL pending passive effect destroy functions are called
// before ANY passive effect create functions are called.
// Otherwise effects in sibling components might 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.
// Layout effects have the same constraint.

// First pass: Destroy stale passive effects.
let unmountEffects = pendingPassiveHookEffectsUnmount;
pendingPassiveHookEffectsUnmount = [];
for (let i = 0; i < unmountEffects.length; i += 2) {
const effect = ((unmountEffects[i]: any): HookEffect);
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;
if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
invokeGuardedCallback(null, destroy, null);
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
destroy();
} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
}
}
}
}
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
}

// Note: This currently assumes there are no passive effects on the root
// fiber, because the root is not part of its own effect list. This could
// change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
// Second pass: Create new passive effects.
let mountEffects = pendingPassiveHookEffectsMount;
pendingPassiveHookEffectsMount = [];
for (let i = 0; i < mountEffects.length; i += 2) {
const effect = ((mountEffects[i]: any): HookEffect);
const fiber = ((mountEffects[i + 1]: any): Fiber);
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
const create = effect.create;
effect.destroy = create();
} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
}
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
}
} else {
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
}
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}

if (enableSchedulerTracing) {
Expand Down
86 changes: 79 additions & 7 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

it('unmounts all previous effects between siblings before creating any new ones', () => {
function Counter({count, label}) {
useEffect(() => {
Scheduler.unstable_yieldValue(`Mount ${label} [${count}]`);
return () => {
Scheduler.unstable_yieldValue(`Unmount ${label} [${count}]`);
};
});
return <Text text={`${label} ${count}`} />;
}
act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="A" count={0} />
<Counter label="B" count={0} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['A 0', 'B 0', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('A 0'), span('B 0')]);
});

expect(Scheduler).toHaveYielded(['Mount A [0]', 'Mount B [0]']);

act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="A" count={1} />
<Counter label="B" count={1} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['A 1', 'B 1', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('A 1'), span('B 1')]);
});
expect(Scheduler).toHaveYielded([
'Unmount A [0]',
'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
]);

act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="B" count={2} />
<Counter label="C" count={0} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['B 2', 'C 0', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('B 2'), span('C 0')]);
});
expect(Scheduler).toHaveYielded([
'Unmount A [1]',
'Unmount B [1]',
'Mount B [2]',
'Mount C [0]',
]);
});

it('handles errors on mount', () => {
function Counter(props) {
useEffect(() => {
Expand Down Expand Up @@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => {
return () => {
Scheduler.unstable_yieldValue('Oops!');
throw new Error('Oops!');
// eslint-disable-next-line no-unreachable
Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`);
};
});
useEffect(() => {
Expand All @@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => {
});
return <Text text={'Count: ' + props.count} />;
}

act(() => {
ReactNoop.render(<Counter count={0} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
Expand All @@ -1679,18 +1739,30 @@ describe('ReactHooksWithNoopRenderer', () => {
});

act(() => {
// This update will trigger an error
// This update will trigger an error during passive effect unmount
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
expect(Scheduler).toHaveYielded(['Oops!']);

// This tests enables a feature flag that flushes all passive destroys in a
// separate pass before flushing any passive creates.
// A result of this two-pass flush is that an error thrown from unmount does
// not block the subsequent create functions from being run.
expect(Scheduler).toHaveYielded([
'Oops!',
'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
]);
});
// B unmounts even though an error was thrown in the previous effect
// B's destroy function runs later on unmount though, since it's passive
expect(Scheduler).toHaveYielded(['Unmount B [0]']);

// <Counter> gets unmounted because an error is thrown above.
// The remaining destroy functions are run later on unmount, since they're passive.
// In this case, one of them throws again (because of how the test is written).
expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']);
expect(ReactNoop.getChildren()).toEqual([]);
});

Expand Down

0 comments on commit f727803

Please sign in to comment.