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.
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
Potential memory leak when navigating between pages #12198
Comments
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/packages/gatsby/package.json Lines 164 to 167 in 3b3153d
I’m thinking of trying locally to update Gatsby to @DSchau What do you think? Is there any reason for Gatsby to use |
@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 Thanks again for being part of the Gatsby community! |
This should be reopened and properly dealt? instead of auto closed by a bot. |
I think this should not closed by a bot.I had the same problem. |
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. |
The bot should not close rgis issue since it is still bringing problems.
…On Sat, Aug 10, 2019, 00:35 atomiks ***@***.***> wrote:
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.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#12198?email_source=notifications&email_token=AABS6LMM3YEFDVEVJL5NVBTQDYZQFA5CNFSM4G3BDVC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4AFBBI#issuecomment-520114309>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABS6LNCAXRFELE74OE7N6LQDYZQFANCNFSM4G3BDVCQ>
.
|
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. |
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 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! |
…t to prevent memory leaks Associated issue: #12198
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 Learned a bunch while following / investigating this issue, thanks everyone (and particularly @atomiks 🙂) Note: this PR only fixes the |
* fix(gatsby-link) Clean up IntersectionObserver on componentWillUnmount to prevent memory leaks Associated issue: #12198 * Check whether IntersectionObserver exists before unobserving
…#17056) * fix(gatsby-link) Clean up IntersectionObserver on componentWillUnmount to prevent memory leaks Associated issue: gatsbyjs#12198 * Check whether IntersectionObserver exists before unobserving
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.
Expected result
JS heap doesn't keep growing when navigating around, esp. already visited pages?
Actual result
Continues to grow
The text was updated successfully, but these errors were encountered: