Flush all passive destroy fns before calling create fns #17947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
bvaughn
commented
Jan 31, 2020
| effect.nextEffect = null; | ||
| effect = nextNextEffect; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This is the old code path. If we flip the deferPassiveEffectCleanupDuringUnmount killswitch off, it will restore the previous behavior.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9ca1b2d:
|
Details of bundled changes.Comparing: 529e58a...9ca1b2d react-art
react-native-renderer
react-dom
react-reconciler
react-test-renderer
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
Details of bundled changes.Comparing: 529e58a...9ca1b2d react-native-renderer
react-test-renderer
react-reconciler
react-dom
react-art
ReactDOM: size: 0.0%, gzip: -0.1% Size changes (experimental) |
2 tasks
bvaughn
force-pushed
the
passive-effect-follow-up
branch
from
fa54752
to
0fb7bba
Compare
|
Rebased after merging PR #17925. |
acdlite
reviewed
Feb 3, 2020
| // Don't run create effects for a Fiber that errored during destroy. | ||
| // This check is in place to match previous behavior. | ||
| // TODO: Rethink whether we want to carry this behavior forward. | ||
| if (effectWithErrorDuringUnmount !== effect) { |
There was a problem hiding this comment.
If multiple fibers errored, this check only works for the last one. So it doesn't seem worth it.
There was a problem hiding this comment.
Fair enough. I would prefer just not carrying this behavior forward anyway. I'll remove it.
|
I was thinking the way you could do this is by maintaining two arrays: one for destroy effects, another for create/init effects. During the sync commit phase, traverse over the hook effect list and push to the appropriate array. Then during The advantage of this approach is it's less total work because in the passive phase, you no longer have to visit the entire effect list. The disadvantage is it sometimes shifts more work to the layout phase, because you can't skip over a fiber that only has passive effects scheduled on it. But the other advantage is that it saves on code size, which is why I'm leaning toward always pushing to an array. cc @sebmarkbage for input. |
|
That could bring things more inline with the way I'm deferring destroy functions on unmount... (That could just be handled by this mechanism as well.) I'm open to making this change, but I'm going to hold off to see what @sebmarkbage thinks since it would require a bit of code juggling (especially since we want to keep everything behind a feature flag for now). Hm...I think we'd need to track the effect's corresponding fiber too, for error tracking purposes. |
|
@acdlite I took a pass at this locally (diff below). It has an added benefit of calling more unmount functions in the case of one throwing, but in order to maintain the expected error behavior- I think it would also be a bit more code than maybe you expected? (We'd have to track the hook effect + Fiber, unless I'm overlooking something.) (Happy to push this commit though if you think the approach is better or if it would be easier to review than a diff.) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
index c6fc36be3..5eeb7d290 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -110,7 +110,8 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
- enqueuePendingPassiveEffectDestroyFn,
+ enqueuePendingPassiveHookEffectMount,
+ enqueuePendingPassiveHookEffectUnmount,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
@@ -396,49 +397,39 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
}
}
-export function commitPassiveHookEffects(finishedWork: Fiber): void {
- if ((finishedWork.effectTag & Passive) !== NoEffect) {
- switch (finishedWork.tag) {
- case FunctionComponent:
- case ForwardRef:
- case SimpleMemoComponent:
- case Chunk: {
- // TODO (#17945) We should call all passive destroy functions (for all fibers)
- // before calling any create functions. The current approach only serializes
- // these for a single fiber.
- commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
- commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
- break;
- }
- default:
- break;
+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 commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
+export function commitPassiveHookEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
+ // TODO (#17945) We should call all passive destroy functions (for all fibers)
+ // before calling any create functions. The current approach only serializes
+ // these for a single fiber.
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
- break;
- }
- default:
- break;
- }
- }
-}
-
-export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
- if ((finishedWork.effectTag & Passive) !== NoEffect) {
- switch (finishedWork.tag) {
- case FunctionComponent:
- case ForwardRef:
- case SimpleMemoComponent:
- case Chunk: {
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
break;
}
@@ -464,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: {
@@ -806,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);
}
diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
index fdb0875f8..851e22f8f 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -144,7 +144,7 @@ export type Hook = {|
next: Hook | null,
|};
-type Effect = {|
+export type Effect = {|
tag: HookEffectTag,
create: () => (() => void) | void,
destroy: (() => void) | void,
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
index ae4db267b..b015e63de 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -14,7 +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 {Hook} from './ReactFiberHooks';
+import type {Effect as HookEffect, Hook} from './ReactFiberHooks';
import {
warnAboutDeprecatedLifecycles,
@@ -133,8 +133,6 @@ import {
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
commitLifeCycles as commitLayoutEffectOnFiber,
commitPassiveHookEffects,
- commitPassiveHookUnmountEffects,
- commitPassiveHookMountEffects,
commitPlacement,
commitWork,
commitDeletion,
@@ -260,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,
@@ -2180,11 +2179,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, () => {
@@ -2195,6 +2211,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
}
}
+function invokePassiveEffectCreate(effect: HookEffect): void {
+ const create = effect.create;
+ effect.destroy = create();
+}
+
function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
@@ -2213,18 +2234,6 @@ 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);
- }
- pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
-
// 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.
@@ -2233,70 +2242,60 @@ function flushPassiveEffectsImpl() {
// Layout effects have the same constraint.
// First pass: Destroy stale passive effects.
- //
- // 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,
- commitPassiveHookUnmountEffects,
- null,
- effect,
- );
- if (hasCaughtError()) {
- invariant(effect !== null, 'Should be working on an effect.');
- const error = clearCaughtError();
- captureCommitPhaseError(effect, error);
- }
- resetCurrentDebugFiberInDEV();
- } else {
- try {
- commitPassiveHookUnmountEffects(effect);
- } catch (error) {
- invariant(effect !== null, 'Should be working on an effect.');
- captureCommitPhaseError(effect, error);
+ 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;
+ if (typeof destroy === 'function') {
+ if (__DEV__) {
+ setCurrentDebugFiberInDEV(fiber);
+ invokeGuardedCallback(null, destroy, null);
+ effect.destroy = undefined;
+ 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);
+ } finally {
+ effect.destroy = undefined;
+ }
}
}
- effect = effect.nextEffect;
}
// Second pass: Create new passive effects.
- //
- // 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.
- effect = root.current.firstEffect;
- while (effect !== null) {
+ 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(effect);
- invokeGuardedCallback(
- null,
- commitPassiveHookMountEffects,
- null,
- effect,
- );
+ setCurrentDebugFiberInDEV(fiber);
+ invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
if (hasCaughtError()) {
- invariant(effect !== null, 'Should be working on an effect.');
+ invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
- captureCommitPhaseError(effect, error);
+ captureCommitPhaseError(fiber, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
- commitPassiveHookMountEffects(effect);
+ const create = effect.create;
+ effect.destroy = create();
} catch (error) {
- invariant(effect !== null, 'Should be working on an effect.');
- captureCommitPhaseError(effect, error);
+ invariant(fiber !== null, 'Should be working on an effect.');
+ captureCommitPhaseError(fiber, error);
}
}
- const nextNextEffect = effect.nextEffect;
- // Remove nextEffect pointer to assist GC
- effect.nextEffect = null;
- effect = nextNextEffect;
}
} else {
// Note: This currently assumes there are no passive effects on the root fiber
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
index cc3eee28d..438a748fb 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -1753,6 +1753,7 @@ describe('ReactHooksWithNoopRenderer', () => {
// not block the subsequent create functions from being run.
expect(Scheduler).toHaveYielded([
'Oops!',
+ 'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
]); |
|
I'm confused about the review process here. The previous PR includes everything from this PR and is merged. #17925 |
Ugh, the rebase history got messed up somehow. This PR builds on top of the previous PR (not the other way around). I'll clean up the history now, but the file change/delta is accurate here. |
bvaughn
force-pushed
the
passive-effect-follow-up
branch
from
44e2b96
to
41e0752
Compare
|
Okay @sebmarkbage @acdlite I manually cleaned up the branch history. (Sorry for the rebase confusion.) There are two commits here now: |
bvaughn
commented
Feb 4, 2020
| effect: HookEffect, | ||
| ): void { | ||
| if (deferPassiveEffectCleanupDuringUnmount) { | ||
| pendingPassiveHookEffectsMount.push(effect, fiber); |
There was a problem hiding this comment.
Retaining both the effect and Fiber is kind of gross, particularly in the same array. I think we'd need to track them both somewhere though because of how we track and report errors.
bvaughn
commented
Feb 4, 2020
| // not block the subsequent create functions from being run. | ||
| expect(Scheduler).toHaveYielded([ | ||
| 'Oops!', | ||
| 'Unmount B [0]', |
There was a problem hiding this comment.
This is the main positive change wrt 41e0752.
|
Ping~ |
sebmarkbage
approved these changes
Feb 11, 2020
|
Thanks for the heads up, Seb. Andrew and I chatted a little offline about this yesterday too, so I think I'm at least aware of some of the planned changes. Happy to help with the refactoring when it's time. |
bvaughn
force-pushed
the
passive-effect-follow-up
branch
from
41e0752
to
674bfc4
Compare
added 2 commits
February 11, 2020 09:32
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).
This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored.
This was referenced Mar 15, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
deferPassiveEffectCleanupDuringUnmountkill switch to separate passive effects flushing into two separate phases (similar to layout effects).It also carries forward a behavior of not running mount functions for a Fiber that's had an unmount function error. Open to discussion on whether this behavior is something we want to carry forward.The
deferPassiveEffectCleanupDuringUnmountflag still remains off in master. We'll want to flip it on for Facebook during an upcoming sync.Resolves issue #17945. Builds on top of PR #17925.