Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flush useEffect clean up functions in the passive effects phase #17925

Merged
merged 6 commits into from
Feb 3, 2020
Merged

Flush useEffect clean up functions in the passive effects phase #17925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1f8379
Flush useEffect clean up functions in the passive effects phase
Jan 29, 2020
cf44b09
Avoid scheduling unnecessary callbacks for cleanup effects
Jan 30, 2020
5f3a3b9
Updated newly added test for added clarity.
Jan 30, 2020
e8c7da1
Cleaned up hooks effect tags
Jan 31, 2020
d93a76f
Disabled deferred passive effects flushing in OSS builds for now
Feb 3, 2020
606af56
Split up unmount and mount effects list traversal
Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 77 additions & 44 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
import {
deferPassiveEffectCleanupDuringUnmount,
enableSchedulerTracing,
enableProfilerTimer,
enableSuspenseServerRenderer,
Expand Down Expand Up @@ -109,16 +110,13 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
enqueuePendingPassiveEffectDestroyFn,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
UnmountSnapshot,
UnmountMutation,
MountMutation,
UnmountLayout,
MountLayout,
UnmountPassive,
MountPassive,
HasEffect as HookHasEffect,
Layout as HookLayout,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
Expand Down Expand Up @@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a snapshot phase hook (yet) so this call was walking the effect list for no reason. I've removed it.

return;
}
case ClassComponent: {
Expand Down Expand Up @@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
);
}

function commitHookEffectList(
unmountTag: number,
mountTag: number,
finishedWork: Fiber,
) {
function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
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 {
if ((effect.tag & unmountTag) !== NoHookEffect) {
if ((effect.tag & tag) === tag) {
// Unmount
const destroy = effect.destroy;
effect.destroy = undefined;
if (destroy !== undefined) {
destroy();
}
}
if ((effect.tag & mountTag) !== NoHookEffect) {
effect = effect.next;
} while (effect !== firstEffect);
}
}

function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
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 {
if ((effect.tag & tag) === tag) {
// Mount
const create = effect.create;
effect.destroy = create();
Expand Down Expand Up @@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
// 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:
Expand All @@ -419,7 +427,11 @@ function commitLifeCycles(
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
// At this point layout effects have already been destroyed (during mutation phase).
// This is done to prevent sibling component effects from interfering with each other,
// 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);
return;
}
case ClassComponent: {
Expand Down Expand Up @@ -756,32 +768,47 @@ function commitUnmount(
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

// When the owner fiber is deleted, the destroy function of a passive
// effect hook is called during the synchronous commit phase. This is
// a concession to implementation complexity. Calling it in the
// passive effect phase (like they usually are, when dependencies
// change during an update) would require either traversing the
// children of the deleted fiber again, or including unmount effects
// as part of the fiber effect list.
//
// Because this is during the sync commit phase, we need to change
// the priority.
//
// TODO: Reconsider this implementation trade off.
const priorityLevel =
renderPriorityLevel > NormalPriority
? NormalPriority
: renderPriorityLevel;
runWithPriority(priorityLevel, () => {
if (deferPassiveEffectCleanupDuringUnmount) {
let effect = firstEffect;
do {
const destroy = effect.destroy;
const {destroy, tag} = effect;
if (destroy !== undefined) {
safelyCallDestroy(current, destroy);
if ((tag & HookPassive) !== NoHookEffect) {
enqueuePendingPassiveEffectDestroyFn(destroy);
} else {
safelyCallDestroy(current, destroy);
}
}
effect = effect.next;
} while (effect !== firstEffect);
});
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the old code path, in case we enable the kill switch.

// When the owner fiber is deleted, the destroy function of a passive
// effect hook is called during the synchronous commit phase. This is
// a concession to implementation complexity. Calling it in the
// passive effect phase (like they usually are, when dependencies
// change during an update) would require either traversing the
// children of the deleted fiber again, or including unmount effects
// as part of the fiber effect list.
//
// Because this is during the sync commit phase, we need to change
// the priority.
//
// TODO: Reconsider this implementation trade off.
const priorityLevel =
renderPriorityLevel > NormalPriority
? NormalPriority
: renderPriorityLevel;
runWithPriority(priorityLevel, () => {
let effect = firstEffect;
do {
const destroy = effect.destroy;
if (destroy !== undefined) {
safelyCallDestroy(current, destroy);
}
effect = effect.next;
} while (effect !== firstEffect);
});
}
}
}
return;
Expand Down Expand Up @@ -1285,9 +1312,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
// 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.
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
return;
}
case Profiler: {
Expand Down Expand Up @@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
// 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.
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
return;
}
case ClassComponent: {
Expand Down
46 changes: 22 additions & 24 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,9 @@ import {
Passive as PassiveEffect,
} from 'shared/ReactSideEffectTags';
import {
NoEffect as NoHookEffect,
UnmountMutation,
MountLayout,
UnmountPassive,
MountPassive,
HasEffect as HookHasEffect,
Layout as HookLayout,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {
scheduleWork,
Expand Down Expand Up @@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
const hook = mountWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
currentlyRenderingFiber.effectTag |= fiberEffectTag;
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
hook.memoizedState = pushEffect(
HookHasEffect | hookEffectTag,
create,
undefined,
nextDeps,
);
}

function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
Expand All @@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
pushEffect(hookEffectTag, create, destroy, nextDeps);
return;
}
}
}

currentlyRenderingFiber.effectTag |= fiberEffectTag;

hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
hook.memoizedState = pushEffect(
HookHasEffect | hookEffectTag,
create,
destroy,
nextDeps,
);
}

function mountEffect(
Expand All @@ -960,7 +968,7 @@ function mountEffect(
}
return mountEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
HookPassive,
create,
deps,
);
Expand All @@ -978,7 +986,7 @@ function updateEffect(
}
return updateEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
HookPassive,
create,
deps,
);
Expand All @@ -988,24 +996,14 @@ function mountLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return mountEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
create,
deps,
);
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
}

function updateLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
create,
deps,
);
return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
}

function imperativeHandleEffect<T>(
Expand Down Expand Up @@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(

return mountEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
Expand All @@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(

return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
Expand Down
31 changes: 31 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
deferPassiveEffectCleanupDuringUnmount,
enableUserTimingAPI,
enableSuspenseServerRenderer,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
Expand Down Expand Up @@ -257,6 +258,7 @@ let rootDoesHavePassiveEffects: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];

let rootsWithPendingDiscreteUpdates: Map<
FiberRoot,
Expand Down Expand Up @@ -2176,6 +2178,21 @@ export function flushPassiveEffects() {
}
}

export function enqueuePendingPassiveEffectDestroyFn(
destroy: () => void,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
flushPassiveEffects();
return null;
});
}
}
}

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
Expand All @@ -2193,6 +2210,20 @@ function flushPassiveEffectsImpl() {
executionContext |= CommitContext;
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);
Copy link
Contributor Author

@bvaughn bvaughn Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop approach has an unintentional side effect of being "better" than our normal destroy approach, since an error thrown by one destroy function won't prevent other destroy functions on the same Fiber from being called.

Copy link
Collaborator

@acdlite acdlite Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you confirm that with a test? I would expect the current behavior to also unmount all the destroy functions on a fiber, even if one throws, because if one of them throws, then the nearest error boundary fiber gets deleted, which triggers commitNestedUnmounts, which then calls the remaining clean-up functions and wraps each call in a try catch:

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

Line 780 in 38cd758

safelyCallDestroy(current, destroy);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes I think you are right. Disregard!

}
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.
Expand Down
16 changes: 8 additions & 8 deletions packages/react-reconciler/src/ReactHookEffectTags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@

export type HookEffectTag = number;

export const NoEffect = /* */ 0b00000000;
export const UnmountSnapshot = /* */ 0b00000010;
export const UnmountMutation = /* */ 0b00000100;
export const MountMutation = /* */ 0b00001000;
export const UnmountLayout = /* */ 0b00010000;
export const MountLayout = /* */ 0b00100000;
export const MountPassive = /* */ 0b01000000;
export const UnmountPassive = /* */ 0b10000000;
export const NoEffect = /* */ 0b000;

// Represents whether effect should fire.
export const HasEffect = /* */ 0b001;

// Represents the phase in which the effect (not the clean-up) fires.
export const Layout = /* */ 0b010;
export const Passive = /* */ 0b100;
Loading