@samitouri / QOS-React-2 / commits / f7278034de

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.

Brian Vaughn committed Feb 11, 2020 at 09:52 UTC f7278034de5a289571f26666e6717c4df9f519ad

4 files changed +221 -48

packages/react-reconciler/src/ReactFiberCommitWork.js

+29 -2

@@ -110,7 +110,8 @@ import {

110 captureCommitPhaseError,

111 resolveRetryThenable,

112 markCommitTimeOfFallback,

113 - enqueuePendingPassiveEffectDestroyFn,

113 + enqueuePendingPassiveHookEffectMount,

114 + enqueuePendingPassiveHookEffectUnmount,

115 } from './ReactFiberWorkLoop';

116 import {

117 NoEffect as NoHookEffect,

@@ -396,6 +397,28 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {

397 }

398 }

399

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 +

422 export function commitPassiveHookEffects(finishedWork: Fiber): void {

423 if ((finishedWork.effectTag & Passive) !== NoEffect) {

424 switch (finishedWork.tag) {

@@ -432,6 +455,10 @@ function commitLifeCycles(

455 // e.g. a destroy function in one component should never override a ref set

456 // by a create function in another component during the same commit.

457 commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);

458 +

459 + if (deferPassiveEffectCleanupDuringUnmount) {

460 + schedulePassiveEffects(finishedWork);

461 + }

462 return;

463 }

464 case ClassComponent: {

@@ -774,7 +801,7 @@ function commitUnmount(

801 const {destroy, tag} = effect;

802 if (destroy !== undefined) {

803 if ((tag & HookPassive) !== NoHookEffect) {

777 - enqueuePendingPassiveEffectDestroyFn(destroy);

804 + enqueuePendingPassiveHookEffectUnmount(current, effect);

805 } else {

806 safelyCallDestroy(current, destroy);

807 }

packages/react-reconciler/src/ReactFiberHooks.js

+1 -1

@@ -144,7 +144,7 @@ export type Hook = {|

144 next: Hook | null,

145 |};

146

147 -type Effect = {|

147 +export type Effect = {|

148 tag: HookEffectTag,

149 create: () => (() => void) | void,

150 destroy: (() => void) | void,

packages/react-reconciler/src/ReactFiberWorkLoop.js

+112 -38

@@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

14 import type {Interaction} from 'scheduler/src/Tracing';

15 import type {SuspenseConfig} from './ReactFiberSuspenseConfig';

16 import type {SuspenseState} from './ReactFiberSuspenseComponent';

17 +import type {Effect as HookEffect} from './ReactFiberHooks';

19 import {

20 warnAboutDeprecatedLifecycles,

@@ -257,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;

258 let rootWithPendingPassiveEffects: FiberRoot | null = null;

259 let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;

260 let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;

260 -let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];

261 +let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];

262 +let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];

263

264 let rootsWithPendingDiscreteUpdates: Map<

265 FiberRoot,

@@ -2168,11 +2170,28 @@ export function flushPassiveEffects() {

2170 }

2171 }

2172

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,

2192 ): void {

2193 if (deferPassiveEffectCleanupDuringUnmount) {

2175 - pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);

2194 + pendingPassiveHookEffectsUnmount.push(effect, fiber);

2195 if (!rootDoesHavePassiveEffects) {

2196 rootDoesHavePassiveEffects = true;

2197 scheduleCallback(NormalPriority, () => {

@@ -2183,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(

2202 }

2203 }

2204

2205 +function invokePassiveEffectCreate(effect: HookEffect): void {

2206 + const create = effect.create;

2207 + effect.destroy = create();

2208 +}

2209 +

2210 function flushPassiveEffectsImpl() {

2211 if (rootWithPendingPassiveEffects === null) {

2212 return false;

@@ -2201,45 +2225,95 @@ function flushPassiveEffectsImpl() {

2225 const prevInteractions = pushInteractions(root);

2226

2227 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 + }

2262 }

2214 - pendingUnmountedPassiveEffectDestroyFunctions.length = 0;

2215 - }

2263

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 + }

2287 }

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 + }

2311 }

2312 + const nextNextEffect = effect.nextEffect;

2313 + // Remove nextEffect pointer to assist GC

2314 + effect.nextEffect = null;

2315 + effect = nextNextEffect;

2316 }

2239 - const nextNextEffect = effect.nextEffect;

2240 - // Remove nextEffect pointer to assist GC

2241 - effect.nextEffect = null;

2242 - effect = nextNextEffect;

2317 }

2318

2319 if (enableSchedulerTracing) {

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

+79 -7

@@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => {

1558 ]);

1559 });

1560

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 +

1622 it('handles errors on mount', () => {

1623 function Counter(props) {

1624 useEffect(() => {

@@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => {

1717 return () => {

1718 Scheduler.unstable_yieldValue('Oops!');

1719 throw new Error('Oops!');

1659 - // eslint-disable-next-line no-unreachable

1660 - Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`);

1720 };

1721 });

1722 useEffect(() => {

@@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => {

1727 });

1728 return <Text text={'Count: ' + props.count} />;

1729 }

1730 +

1731 act(() => {

1732 ReactNoop.render(<Counter count={0} />, () =>

1733 Scheduler.unstable_yieldValue('Sync effect'),

@@ -1679,18 +1739,30 @@ describe('ReactHooksWithNoopRenderer', () => {

1739 });

1740

1741 act(() => {

1682 - // This update will trigger an error

1742 + // This update will trigger an error during passive effect unmount

1743 ReactNoop.render(<Counter count={1} />, () =>

1744 Scheduler.unstable_yieldValue('Sync effect'),

1745 );

1746 expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);

1747 expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);

1748 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 + ]);

1760 });

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]']);

1766 expect(ReactNoop.getChildren()).toEqual([]);

1767 });

1768