Flush `useEffect` clean up functions in the passive effects phase #17925
Conversation
| } | ||
| effect = effect.next; | ||
| } while (effect !== firstEffect); | ||
| }); | ||
| } else { |
bvaughn
Jan 29, 2020
Author
Contributor
This is the old code path, in case we enable the kill switch.
This is the old code path, in case we enable the kill switch.
| // Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation. | ||
| // This is to ensure ALL destroy fns are run before create fns, | ||
| // without requiring us to traverse the effects list an extra time during commit. | ||
| // This sequence prevents sibling destroy and create fns from interfering with each other. |
bvaughn
Jan 29, 2020
Author
Contributor
I added these comments for myself, since it wasn't clear to me why we were using these tags.
I added these comments for myself, since it wasn't clear to me why we were using these tags.
| export const MountLayout = /* */ 0b000100000; | ||
| export const MountPassive = /* */ 0b001000000; | ||
| export const UnmountPassive = /* */ 0b010000000; | ||
| export const NoEffectPassiveUnmountFiber = /**/ 0b100000000; |
bvaughn
Jan 29, 2020
Author
Contributor
The name of the new effect tag I added, NoEffectPassiveUnmountFiber, is kind of stupid. I'd welcome a solution for a better name. 🤪
The name of the new effect tag I added, NoEffectPassiveUnmountFiber, is kind of stupid. I'd welcome a solution for a better name.
acdlite
Jan 30, 2020
•
Member
I think it should be possible to simplify these tags by removing the Mount/Unmount . I think that's already handled because commitHookEffectList accepts two separate arguments for which tags to mount and which tags to unmount.
Would it work if the only tags we had were this?
// Represents whether effect should fire.
export const HasEffect = /* */ 0b0001;
// Represents the phase in which the effect (not the clean-up) fires.
export const Snapshot = /* */ 0b0010;
export const Layout = /* */ 0b0100;
export const Passive = /* */ 0b1000;
So during an update you would fire the clean up of any hook that has (for passive effects) that has Passive | HasEffect. When the fiber is deleted, you would only check Passive.
I think it should be possible to simplify these tags by removing the Mount/Unmount . I think that's already handled because commitHookEffectList accepts two separate arguments for which tags to mount and which tags to unmount.
Would it work if the only tags we had were this?
// Represents whether effect should fire.
export const HasEffect = /* */ 0b0001;
// Represents the phase in which the effect (not the clean-up) fires.
export const Snapshot = /* */ 0b0010;
export const Layout = /* */ 0b0100;
export const Passive = /* */ 0b1000;So during an update you would fire the clean up of any hook that has (for passive effects) that has Passive | HasEffect. When the fiber is deleted, you would only check Passive.
bvaughn
Jan 31, 2020
•
Author
Contributor
I think this cleanup suggestion makes sense, although while looking into it I realized we have a potential bug with the sequence/timing of passive effects. (We don't call all destroy functions before create functions.) I'll take a pass at the effect tag cleanup and separating the passive effect destroy/create phases too. If it starts looking like that might be a bigger change though, I may split it out into a follow up PR (#17945)
I think this cleanup suggestion makes sense, although while looking into it I realized we have a potential bug with the sequence/timing of passive effects. (We don't call all destroy functions before create functions.) I'll take a pass at the effect tag cleanup and separating the passive effect destroy/create phases too. If it starts looking like that might be a bigger change though, I may split it out into a follow up PR (#17945)
bvaughn
force-pushed the
bvaughn:defer-passive-effect-cleanup-during-unmount
branch
from
bf7b3c3
to
50f1706
| function updateEffectImpl( | ||
| fiberEffectTag, | ||
| hookEffectTag, | ||
| noWorkUnmountFiberHookEffectTag, |
bvaughn
Jan 29, 2020
Author
Contributor
This name isn't the greatest either. I welcome better suggestions.
The purpose of the changes to this function is to retain the ability to distinguish between layout and passive effects even if a render has no dependency changes. Previously, we would not be able to distinguish between the two during unmount.
This name isn't the greatest either. I welcome better suggestions.
The purpose of the changes to this function is to retain the ability to distinguish between layout and passive effects even if a render has no dependency changes. Previously, we would not be able to distinguish between the two during unmount.
|
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 bf7b3c3:
|
|
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 606af56:
|
Details of bundled changes.Comparing: 434770c...606af56 react
react-dom
react-test-renderer
react-native-renderer
react-art
react-reconciler
scheduler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 434770c...606af56 react
react-dom
react-native-renderer
react-reconciler
react-art
scheduler
react-test-renderer
ReactDOM: size: Size changes (stable) |
| // HACK | ||
| // This update is kind of gross since it exists to test an internal implementation detail: | ||
| // Effects without updating dependencies lose their layout/passive tag during an update. | ||
| // A special type of no-update tag (NoEffectPassiveUnmountFiber) is used to track these for later. |
acdlite
Jan 30, 2020
Member
To make it less "hacky" you could include the count in the log message. So that the effect actually depends on it.
Then make the other effect close over a different prop. count2 or whatever.
Then during the update, you change one of the props but not the other. Only that effect should fire; the other one will bail out.
To make it less "hacky" you could include the count in the log message. So that the effect actually depends on it.
Then make the other effect close over a different prop. count2 or whatever.
Then during the update, you change one of the props but not the other. Only that effect should fire; the other one will bail out.
bvaughn
Jan 30, 2020
Author
Contributor
Yeah, sure. That"hacky" part is that I'm only doing this intermediate update because I know that used to cause the specific implementation to blur the passive and layout effects together.
Yeah, sure. That"hacky" part is that I'm only doing this intermediate update because I know that used to cause the specific implementation to blur the passive and layout effects together.
| if (deferPassiveEffectCleanupDuringUnmount) { | ||
| pendingUnmountedPassiveEffectDestroyFunctions.push(destroy); | ||
| rootDoesHavePassiveEffects = true; | ||
| scheduleCallback(NormalPriority, () => { |
acdlite
Jan 30, 2020
Member
This will schedule a callback for every clean-up effect. Need to check if rootDoesHavePassiveEffects is already true.
This will schedule a callback for every clean-up effect. Need to check if rootDoesHavePassiveEffects is already true.
bvaughn
Jan 30, 2020
Author
Contributor
Ah, good call!
Ah, good call!
bvaughn
force-pushed the
bvaughn:defer-passive-effect-cleanup-during-unmount
branch
from
50f1706
to
d140ab0
bvaughn
force-pushed the
bvaughn:defer-passive-effect-cleanup-during-unmount
branch
from
d140ab0
to
1c6be34
| @@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles( | |||
| case ForwardRef: | |||
| case SimpleMemoComponent: | |||
| case Chunk: { | |||
| commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork); | |||
bvaughn
Jan 31, 2020
Author
Contributor
We don't have a snapshot phase hook (yet) so this call was walking the effect list for no reason. I've removed it.
We don't have a snapshot phase hook (yet) so this call was walking the effect list for no reason. I've removed it.
| i++ | ||
| ) { | ||
| const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i]; | ||
| invokeGuardedCallback(null, destroy, null); |
bvaughn
Jan 31, 2020
•
Author
Contributor
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.
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.
acdlite
Jan 31, 2020
•
Member
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:
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:
bvaughn
Jan 31, 2020
Author
Contributor
Ah, yes I think you are right. Disregard!
Ah, yes I think you are right. Disregard!
packages/shared/ReactFeatureFlags.js
Outdated
| // Previously these functions were run during commit (along with layout effects). | ||
| // Ideally we should delay these until after commit for performance reasons. | ||
| // This flag provides a killswitch if that proves to break existing code somehow. | ||
| export const deferPassiveEffectCleanupDuringUnmount = true; |
bvaughn
Jan 31, 2020
Author
Contributor
Open question: should I set this to false for now (for open source releases)?
Open question: should I set this to false for now (for open source releases)?
acdlite
Feb 3, 2020
Member
Yeah probably, just to be safe
Yeah probably, just to be safe
bvaughn
Feb 3, 2020
Author
Contributor
K, will do then.
K, will do then.
|
Left some nits. Looks good to me! |
| if ((effect.tag & mountTag) !== NoHookEffect) { | ||
| if ( | ||
| (effect.tag & HookHasEffect) !== NoHookEffect && | ||
| (effect.tag & mountTag) !== NoHookEffect |
acdlite
Feb 3, 2020
Member
Nit: You can save a check here if you assume mountTag and unmountTag already include HookHasEffect:
(effect.tag & mountTag) === mountTag
Nit: You can save a check here if you assume mountTag and unmountTag already include HookHasEffect:
(effect.tag & mountTag) === mountTag
bvaughn
Feb 3, 2020
Author
Contributor
Hmm! But they specifically don't.
We use the tag to specify the type of effect even if there's no work currently required by the hook. This lets us properly differentiate between passive and layout effects in the unmount case. (NoHookEffect is used to signal that there's also work for that specific hook during the current commit.)
Hmm! But they specifically don't.
We use the tag to specify the type of effect even if there's no work currently required by the hook. This lets us properly differentiate between passive and layout effects in the unmount case. (NoHookEffect is used to signal that there's also work for that specific hook during the current commit.)
| // before calling any create functions. The current approach only serializes | ||
| // these for a single fiber. | ||
| commitHookEffectList(HookPassive, NoHookEffect, finishedWork); | ||
| commitHookEffectList(NoHookEffect, HookPassive, finishedWork); |
acdlite
Feb 3, 2020
Member
Following from previous comment, here you would combine the tag with HookHasEffect before passing them in:
commitHookEffectList(HookPassive | HookHasEffect, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, HookPassive | HookHasEffect, finishedWork);
(I assume Closure inlines those values, or you could also hoist them out to module scope)
Following from previous comment, here you would combine the tag with HookHasEffect before passing them in:
commitHookEffectList(HookPassive | HookHasEffect, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, HookPassive | HookHasEffect, finishedWork);(I assume Closure inlines those values, or you could also hoist them out to module scope)
This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)
Updated enqueuePendingPassiveEffectDestroyFn() to check rootDoesHavePassiveEffects before scheduling a new callback. This way we'll only schedule (at most) one.
We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase. This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.
bvaughn
force-pushed the
bvaughn:defer-passive-effect-cleanup-during-unmount
branch
from
1c6be34
to
d93a76f
bvaughn
added a commit
to bvaughn/react
that referenced
this pull request
Feb 21, 2020
I recently landed a change to the timing of passive effect cleanup functions during unmount (see facebook#17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.
bvaughn
added a commit
to bvaughn/react
that referenced
this pull request
Feb 21, 2020
I recently landed a change to the timing of passive effect cleanup functions during unmount (see facebook#17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.
bvaughn
added a commit
that referenced
this pull request
Feb 21, 2020
I recently landed a change to the timing of passive effect cleanup functions during unmount (see #17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.
Merged
This was referenced Dec 3, 2020
Closed
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.
This PR cleans up our hook effect tags and also fixes the case where we were too eagerly flushing passive effect destroy functions on unmount. (As a side effect, we are also now reliably able to distinguish between passive and layout effects during unmount, which will improve the new Profiler methods added in #17910.)
The two major changes in this PR are:
This is a change in behavior/timing for destroy functions so it may cause code to break unexpectedly. For this reason, it has been added behind a killswitch feature flag (
deferPassiveEffectCleanupDuringUnmount).I've also identified some follow up work for passive effects, but to keep this PR small and easy to review, I've just added a TODO for this and will be tracking it with a separate issue, #17945.