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

Clear memoizedState on unmount of fiber to avoid memory leak #14218

Merged
merged 3 commits into from
Nov 19, 2018
Merged

Clear memoizedState on unmount of fiber to avoid memory leak #14218

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f61dd67
Clear memoizedState on unmount of fiber
trueadm Nov 13, 2018
b4c1193
Add other properties on the Fiber object
trueadm Nov 16, 2018
5d65390
remove nextEffect null out
trueadm Nov 16, 2018
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
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,11 +726,20 @@ function detachFiber(current: Fiber) {
// get GC:ed but we don't know which for sure which parent is the current
// one so we'll settle for GC:ing the subtree of this child. This child
// itself will be GC:ed when the parent updates the next time.
// We do not null out the 'nextEffect' field as it causes tests to fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a better understanding of this? "Causes tests to fail" doesn't say anything about the intent and whether those tests should fail or if there's some other mistake.

This feels dangerous to me if we don't understand it. How do you know clearing firstEffect is safe if nextEffect is unsafe? Maybe we just accidentally hit some cases in our tests but not others.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I'd prefer that we don't use vague wording like "causes tests to fail" in the comments. Either we know why it fails and explain the conceptual reason. Or we don't know, and should understand before merging.

Copy link
Contributor Author

@trueadm trueadm Nov 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@gaearon I was more asking @acdlite for some guidance on this. I also reverted the PR, as I also agree that this needs more exploration.

current.return = null;
current.child = null;
current.memoizedState = null;
current.updateQueue = null;
current.firstEffect = null;
current.lastEffect = null;
if (current.alternate) {
current.alternate.child = null;
current.alternate.return = null;
current.alternate.child = null;
current.alternate.memoizedState = null;
current.alternate.updateQueue = null;
current.alternate.firstEffect = null;
current.alternate.lastEffect = null;
}
}

Expand Down