[SSR] Controllers are not garbage collected correctly

[RafalFilipek]

Bug report

Describe the bug

Hi,

I found this bug in my Next.js app, but we think it's not related to NextJS itself but rather SSR in general.

Let's go straight to example:

To Reproduce

• clone this repo - https://github.com/RafalFilipek/nextjs-react-spring-bug
• yarn install
• yarn build
• yarn start

This command will start production version of application with --inspect flag to enable NodeJs debugger.

• open [http://localhost:3000](http://localhost:3000) in Chome
• open DevTools and click NodeJS logo on the left

This will open Dedicated NodeJS debugger.

• open Memory tab, select Heap Snapshot and click Take Snapshot. You will get something like this:

Now it's time to make some requests. I'm using autocannon, but you can use any tool like ab.

autocannon -c 50 -a 1000 http://localhost:3000 

Now you can take second snapshot. It should be much bigger:

After 4K requests you will see that there is a lot of allocations in array part. For some reason there are many allocations of Controller class from react-spring. I have no idea why.

It looks like new Controller class is created for each request, but old instances are not garbage collected. (this and this lines?)

Now let's compare snapshots in this same application but without getInitialProps in _app.js - no SRR.

After 10K request you will get something like this:

• Snapshot 1 - after yarn start
• Snapshot 2 - after 10K req
• Snapshot 3 - after GC

System information

• OS: macOs / Docker 12.4.0-alpine
• Version of Next.js: 9.0.2, 9.0.3
• Version of react-spring 8.0.27, 9.0.0-beta.31
• Version of React: 16.8.6

Thank you

[RafalFilipek]

Same with 9.0.0-beta.33

[RafalFilipek]

@aleclarson I think the problem is that stop function from FrameLoop is not called.

When I add this.controllers.clear(); in start method of FrameLoop there is no leak (I know it's not a solution).

Thank you

[VindulaF]

Running into the same issue here, our Next.js app server ran into OOM issues constantly because of this, we were using react-spring: 8.0.27 and next: 7.0.2.

[RafalFilipek]

@aleclarson is there anything I can try to solve this issue? I know you are quite busy with v9 so maybe some hints :)

Thank you

[aleclarson]

Haven't had time to dig into this yet. You can use issuehunt.io to put a bounty on this issue if it's a blocker for you.

[RafalFilipek]

@aleclarson Thanks, I'll wait for new beta :) I can see you moved controllers Set inside FrameLoop. Maybe it will help.

[DeviousM]

I can confirm that this happens on Razzle & After.js combo and on the newest version of the beta (9.0.0-beta.34).

• Snapshot 1 - Right after node.js server starts
• Snapshot 2 - After first request
• Snapshot 3 - After 2600 requests.

Heap has grown substantially and after few hours on a production server it causes a crash beacuse, quite obviously, OOM.

How far is it from fixing that issue and can we expect it to be fixed in the newest beta?

[aleclarson]

@DeviousM I'm not using react-spring with SSR anywhere right now. Have you tried with the latest canary?

[DeviousM]

Yup, I've just tried the latest canary and got this error:

TypeError: Cannot read property 'ref' of null
    at useTransition (/stonly/apps/stonly-player/stonly/node_modules/@react-spring/core/index.cjs.js:2211:1)
    at /stonly/apps/stonly-player/build/webpack:/stonly-commons/components/HOC/Panel/Panel.js:104:1
    at processChild (/stonly/apps/stonly-player/build/webpack:/stonly/node_modules/react-dom/cjs/react-dom-server.node.development.js:3206:1)
    at resolve (/stonly/apps/stonly-player/build/webpack:/stonly/node_modules/react-dom/cjs/react-dom-server.node.development.js:3126:1)
    at ReactDOMServerRenderer.render (/stonly/apps/stonly-player/build/webpack:/stonly/node_modules/react-dom/cjs/react-dom-server.node.development.js:3600:1)
    at ReactDOMServerRenderer.read (/stonly/apps/stonly-player/build/webpack:/stonly/node_modules/react-dom/cjs/react-dom-server.node.development.js:3538:1)
(...rest is useless)

And the Panel.js component that's throwing that error is actually using the useTransition hook like that:

/* ...rest ommited... */
const positionMap = {
  left: 'translate3d(-100%, 0, 0)',
  right: 'translate3d(100%, 0, 0)',
};

const transition = useTransition(show, null, {
  from: { transform: positionMap[position] },
  enter: { transform: 'translate3d(0,0px,0)' },
  leave: { transform: positionMap[position] },
  config: { mass: 1, tension: 500, friction: 47 },
});
/* ...rest ommited... */

I'll provide a minimal reproduction case in a moment.

[DeviousM]

Alright, @aleclarson, I've created a repo that allows you to clone it and run the heap test locally: https://github.com/DeviousM/react-spring-ssr-issues

To reproduce the issue clone the project, then yarn to install everything and then yarn start to start it up. When you have it running, you can access the node.js's profiler tool in Chrome on localhost:7776. (you can find it at chrome://inspect)
Take a first heap snapshot after the app starts up and you'll see that it's pretty clean.

Then to simulate some load on the server download a tool like siege (that's what I'm testing with) and let it swarm the server for around 45 seconds (if you download siege then commad is siege http://localhost:3005). You can watch live how the server's heap size is growing to end up at least twice as big, mostly clogged by FrameLoop:

Let me know if you have any more questions about my example.
EDIT:// I pushed to the repo, because I kind of forgot to do that one, specific thing 😅

[aleclarson]

Yup, I've just tried the latest canary and got this error:

That's because the function signature of useTransition has changed. More info here: #809

I gave @RafalFilipek's repro a try with both 9.0.0-beta.34 and 9.0.0-canary.808.9.7e75a67.

After 10k requests, it went from 5.2MB to 44.5MB when using 9.0.0-beta.34. After installing the latest canary, it went from 5.2MB to only 8.8MB. There may or may not still be a small leak, but that's a great improvement.

You can see my fork of the repro here: https://github.com/aleclarson/repro/commits/react-spring-778

[DeviousM]

Oh, alight! Let me try it then, I'll report back with results.

[DeviousM]

Yep, it fixes the memory leak! Thanks!

BTW - is there a list of all of the API's that have changed? Because there are quite few places where my app now looks bad (ie. Trail now kind of broke) + is there a way to get index of the item in the new Transition implementation?

[RafalFilipek]

Hi,

I've just tested my example with 9.0.0-canary.808.9.7e75a67. 40K requests later there is no sign o any leak. Some pick around 0.5 MB can be related to internal NodeJS cache system or something like that.

@aleclarson I think we can close this issue. I'll monitor new versions actively :)

Thank you!

[DeviousM]

Yup, on our production build the SSR server didn't grow almost at all.

I agree that we could close the issue, though shouldn't we wait with it until the v9 or at least a new beta is released? Because currently it's still a case for normal react-spring and react-spring@next installation.

[aleclarson]

BTW - is there a list of all of the API's that have changed?

Not yet. I'll write one up when we're closer to v9 being released.

Because there are quite few places where my app now looks bad (ie. Trail now kind of broke) + is there a way to get index of the item in the new Transition implementation?

Could you ask these questions separately in our Spectrum community? Thanks 👍

[v-Bauer]

Hi,
I have already written in another thread with the same bug.
is there a possibility that this bug is fixed in v8? Unfortunately, we can't update to v9beta because of other bugs and the canary version doesn't seem to be suitable for production use yet and doesn't have any documentation.
We would also be willing to pay / donate for the fix in the current stable version. 😊

[topaxi]

With the cause being known that controllers within https://github.com/react-spring/react-spring/blob/66087e18b64d6c6d5fd75f6d84e85ed59714769d/src/animated/FrameLoop.ts#L6 are not being cleaned up, what would we need to get this fixed? I'm up for helping out, but I'm not 100% certain where I should clean this up? Either the Controller class should remove itself when being stopped/destroyed or wherever they are being created they should be cleaned up.

Any help from the maintainers would be great, I'd be happy to help out on a weekend of mine, should I contact you via spectrum?

[KB1RMA]

I commented on Damian's PR as well but I wanted to bring this issue back up again as I've traced a memory leak in my SSR application to the same place using 9.0.0-beta.34

It's certainly an ugly hack, but perhaps that's okay?

[aleclarson]

If anyone has time, please try with 9.0.0-rc.2 and let me know how it goes. 👍

More info: #985

[topaxi]

9.0.0 breaks compatibility and has some issues and last time I tried it, it broke on IE11. A backported fix to version 8.x would be highly appreciated.

[aleclarson]

@topaxi v9 is more stable than v8 at this point. I recommend migrating to it and reporting any bugs you may find. I'll fix 'em up real quick ;)

[joselcc]

It looks like Controllers are not the only ones not being garbage collected

[schriker]

Hi, it seems like i'm having the same issue. Any updates on this?

Stack

• react: 17.0.1
• next.js: 10.0.5
• react-spring: 8.0.27

Edit:

Seems like in my case upgrading react-spring to 9.0.0-rc.3 solved the issue with memory leak.

[joshuaellis]

closing due to inactivity, looks like it was fixed in rc3. please use discussions or discord if you want more help or please consider creating a PR if you want to add a feature 😄