Potential memory leak when navigating between pages
atomiks opened this issue · 22 comments
Description
If this is expected behavior or I'm reading the Memory tab wrong, feel free to close this 😅
What I'm seeing is when navigating between pages (even retracing steps back to an already-visited page), the JS heap size keeps growing.
Steps to reproduce
This happens on both my website and the React site.
- Open https://reactjs.org/
- Open DevTools > Memory > Take Heap Snapshot
- Result: 13.7MB
- Click 'Get Started' and click the 'Main Concepts' subheading on the right. Navigate through every page.
- Take another snapshot > 22.6MB
- Retrace all the steps backwards (already visited pages)
- Take another snapshot > 28.9MB
Expected result
JS heap doesn't keep growing when navigating around, esp. already visited pages?
Actual result
Continues to grow
Interesting!
Yeah - I can reproduce this fairly reliably. First time I tried the heap didn't grow, but second and third times it did 🤷♂️
Will have to dive in to this at some point (or would appreciate any help!) to figure out how we can improve upon this.
Hi!
I’m interested in following-up on this memory leak. Could also reproduce the problem by reproducing the steps given by @atomiks.
It’s even clearer on the Memory Timeline view: each “step” appears when I navigate through another page:
I dug around in the React issues and found facebook/react#14732 which may be of interest (TL;DR a memory leak was present from 16.2.5, fixed in 16.7.0).
It is my understanding that Gatsby uses React 16.4.2 (see the peer dependencies in the gatsby package package.json :
gatsby/packages/gatsby/package.json
Lines 164 to 167 in 3b3153d
I’m thinking of trying locally to update Gatsby to ^16.7.0, link it to and play around with the React website and seeing whether it fixes the memory leak.
@DSchau What do you think? Is there any reason for Gatsby to use react@16.4.2 and not a more recent version, apart from upgrading not being a priority?
@phacks those are peerDependencies, so Gatsby doesn't really tie to a specific version. You can use anything in that range, which would basically be anything less than a major version bump.
However, since this is reactjs.org that we're talking about, we can see that they're on 16.8.3 so while a super good idea--not sure that bumping the peerDependency is the fix here!
@DSchau Indeed, good catch! I’ll try and think of other possible causes, will keep you updated 👍
We do lazy load js bundles and query result json files, so it is expected that memory gets more and more saturated as you navigate the page. Question is if there is some unnecessary memory usage / memory leaks
@pieh Is going back & forth between two pages supposed to generate new queries (and thus memory usage grow)?
Yeah I was thinking visting pages already in the cache shouldn't cause it to continue to grow
Hiya!
This issue has gone quiet. Spooky quiet. 👻
We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
Thanks for being a part of the Gatsby community! 💪💜
Hey again!
It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
Thanks again for being part of the Gatsby community!
This should be reopened and properly dealt? instead of auto closed by a bot.
This should be r
I think this should not closed by a bot.I had the same problem.
The difference is that my data is very large, which makes my page very sluggish
I am willing to work in this issue... but not sure where to start... is this a gatsby bug or a react bug?
I tried again on the React site (I'm assuming it's been updated to 16.9 which fixed that memory leak), problem still persists.
Lots of detached nodes, including Detached IntersectionObserver (I'm assuming that one is Gatsby),and Detached Anchor/Div Elements (looks like React).
In particular, a constructor called "Xl" (minified Fiber node?) is the most problematic and grows infinitely in size and is not cleaning up destroyed nodes properly when navigating to new routes.
Looks to be mostly a React issue with IntersectionObserver being Gatsby.
Oh wow @atomiks, good investigation. We don't clean up IntersectionObservers in our Link component https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-link/src/index.js#L34
Should be an easy fix if someone wants to grab it -- just assign the observer to let scoped to the module and clean it up in componentWillUnmount.
So I'm assuming it doesn't get cleaned up if the callback never gets executed (i.e. never reaches the intersection threshold?)
const createIntersectionObserver = (el, cb) => {
const io = new window.IntersectionObserver(entries => {...})
io.observe(el)
return { instance: io, el }
}
// ...
handleRef() {
// ...
this.io = createIntersectionObserver(ref, () => {
___loader.enqueue(parsePath(this.props.to).pathname)
})
}
componentWillUnmount() {
const {instance, el} = this.io
instance.unobserve(el)
instance.disconnect()
}Reading the memory leak issues/PRs, it mentioned that the user's own code can cause cascading memory leaks in the fiber architecture of React (rather than React itself), so it may be something Gatsby is doing. There is an umbrella issue in the React repo about it.
So I'm assuming it doesn't get cleaned up if the callback never gets executed (i.e. never reaches the intersection threshold?)
Yeah exactly
Hey again!
It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!
Thanks again for being part of the Gatsby community!
Sorry to bother with this issue, but I dont really understand if this is a React issue ( and if so, then I should be looking for the issue in the React repository) or if it is not possible to resolve
@diegodorado It looks like @KyleAMathews suggested a possible fix earlier #12198 (comment), still open for grabs. I can have a look at it this weekend but if anybody wants to take a shot earlier please go ahead!
Pushed a PR (#17056) merely implementing @atomiks’ fix, which I tested out on a local version of https://reactjs.org and is indeed solving the memory leak of IntersectionObservers!
Learned a bunch while following / investigating this issue, thanks everyone (and particularly @atomiks 🙂)
Note: this PR only fixes the IntersectionObserver related memory leak, there are still a bunch of DetachedDOMElements lying around and piling up during navigation.