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.

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

Possible Memory Leak with Styles #10927

Closed
1 task done
lostpebble opened this issue Apr 5, 2018 · 17 comments
Closed
1 task done

Possible Memory Leak with Styles #10927

lostpebble opened this issue Apr 5, 2018 · 17 comments
Assignees

Comments

@lostpebble
Copy link

lostpebble commented Apr 5, 2018

  • I have searched the issues of this repository and believe that this is not a duplicate.

Although there was an issue before about this kind of leak: #8019

That was apparently resolved, so thought I'd open up a new issue if its perhaps something new this time.

Context

I'm using material-ui in my server-rendered React app.

Behavior

I've been searching for a memory leak in my Node.js server lately. It was getting pretty severe, my servers are leaking 100s of MB every day. Through this process I've been doing memory dumps using Chrome dev tools and looking at the object allocations between Snapshots to see what is retained (after many requests and abuse to the server).

What I've found is plenty of retained data of (array) type which seems to be pointing to sheetsManager which is also pointing to material-ui and the withStyles.js file:

image

If I open up one of those sheet things, I get this:

image

Which clearly points to jss and you can see some key which says paperAnchorTop which sounds very much like material-ui.

There are 213 of these entries in the array which have been retained. Which sounds like the same amount of page requests which I made to my server.

The entry to my app on both server and client looks like this:

export function baseReact(stores, router, userAgent) {
  const muiThemeComponents = {};

  // For SSR, user agent passed from request
  if (userAgent) {
    muiThemeComponents.userAgent = userAgent;
  }

  const muiTheme = createMuiTheme();

  return (
    <ErrorBoundary>
      <Provider {...stores}>
        <MuiThemeProvider theme={muiTheme}>{router}</MuiThemeProvider>
      </Provider>
    </ErrorBoundary>
  );
}

Which on the server is rendered like so:

const html = reactDomServer.renderToString(
  baseReact(ctx.state.mobx, <RouterContext {...props} />, userAgent)
);

I think that's pretty stock standard and shouldn't be causing any leaks just like that.

Since the large majority of retained information seems to be coming from this sheet :: StyleSheet object, I think that something is leaking either with jss or the way material-ui deals with styles itself.

Your Environment

Tech Version
Material-UI 1.0.0-beta.40
React 16.3.1
browser Chrome
Node.js 9.8.0
@lostpebble lostpebble changed the title Possible Memory Leak Possible Memory Leak with Styles Apr 5, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 5, 2018

@lostpebble Thanks for the report. We better find the root of the issue. Nothing pops out of my mind with the provided information. Are you able to provide a reproduction repository?

@lostpebble
Copy link
Author

lostpebble commented Apr 5, 2018

Hi @oliviertassinari , cool I've put together a small repo quick. Should hopefully be able to reveal something: https://github.com/lostpebble/material-ui-potential-style-leak

@lostpebble
Copy link
Author

lostpebble commented Apr 7, 2018

This is actually becoming quite a large issue at the moment. My servers are resetting at least twice a day now because of the stagnant memory build-up. And from what I can see, it's something called sheetsManager which is the culprit. I think this is something used by MuiThemeProvider, as I see on the server-rendering guide it's something passed into MuiThemeProvider:

<JssProvider registry={sheetsRegistry} generateClassName={generateClassName}>
   <MuiThemeProvider theme={theme} sheetsManager={new Map()}>
     <App />
   </MuiThemeProvider>
</JssProvider>

I'm not really sure about the internal workings. I'll try dig into that soon. But perhaps MuiThemeProvider needs another prop such as serverRendering which causes sheetsManager to not be persisted (I assume some kind of persistence is needed on the client side) after a single render.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 7, 2018

@lostpebble I just had a look at your reproduction repository. There is definitely a leak. It's easy to fix. I'm looking at how to prevent people from falling into this trap in the first place.

@lostpebble
Copy link
Author

lostpebble commented Apr 7, 2018

Ahh, such a silly little thing for me to overlook. Thanks for the fix @oliviertassinari ! Glad we got to the bottom of it.

I added a sheetsManager prop to MuiThemeProvider in the repo and the leak has disappeared.

@bennidhamma
Copy link

bennidhamma commented Jul 18, 2018

@lostpebble I'm confused by your last comment - in the example code you provide I see sheetsManager={new Map()} but then you say later that you fixed this by "adding a sheetsManager prop to MuiThemeProvider"

But weren't you already doing this?

We're leaking as well, and we are currently putting a new Map() into our MuiThemeProvider sheetsManager prop

@dmdb
Copy link

dmdb commented Apr 14, 2019

I've got same situation in chrome dev tools. Application runs to heap memory limit when under load. Using latest Next.js and material-ui, installed according to https://github.com/mui-org/material-ui/tree/master/examples/nextjs . @oliviertassinari could you advice where to look at?

@eps1lon
Copy link
Member

eps1lon commented Apr 14, 2019

@dmdb Please include the versions you are using where the memory leaks (just base components or own styles using makeStyles, withStyles etc?)

The styling solution does not support strict mode or concurrent mode yet. In those modes styles can leak.

@dmdb
Copy link

dmdb commented Apr 14, 2019

I use this to custommise styles:
import { withStyles } from '@material-ui/core/styles';
And this some of related packages versions:

"@material-ui/core": "^3.9.3",
"@material-ui/icons": "^3.0.2",
"jss": "^9.8.7",
"styled-jsx": "^3.2.0",
"next": "^8.0.4",
"react-jss": "^8.6.1",

Is there all up to date in nextjs integration example? Iam also did'nt get how @lostpebble solved this :(

@dmdb
Copy link

dmdb commented Apr 14, 2019

If there is no known pitfalls about this, I will try to reproduce it in clean repo. Thanks in advance!

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 14, 2019

@dmdb You would need to further isolate the problem. The examples in the /examples folder are up to date.

@dmdb
Copy link

dmdb commented Apr 16, 2019

I've found memory leak in my application, and it was my bad code. Nextjs example got no problems (tested while tried to reproduce :) ) Anyway, sorry and thanks for attention!

@Irrelon
Copy link

Irrelon commented May 10, 2019

@dmdb Hey ya, can you give a little more info on what the leak was? I'm running an almost exact version-by-version same as you for the modules you listed and I've followed the example to implement jss server-side rendering with next.js but I'm seeing lots of retained objects (probably is my code but wanted to check with you)

@kristfal
Copy link

kristfal commented Jun 6, 2019

Hey guys,

we've been running with Mui (1.5.0) for a while and have discovered that Mui and/or styled components leak in a SSR env on redirects.

Our SSR render function was as follows:

export default ({clientStats}) => async (req, res) => {
  const store = await configureStore(req, res);

  const sheet = new ServerStyleSheet();
  const sheetsRegistry = new SheetsRegistry();
  const generateClassName = createGenerateClassName();
  const theme = createMuiTheme();

  if (!store) return null; // no store means redirect was already served

  // More stuff..

Where configureStore would own the logic of calling res.redirect(302, path) and return null in case of a redirect. If there was no redirect the render returned our HTML with styles injected.

Regular requests without triggering redirects did not leak.

However, if there was a redirect, the code above leaked about 500mb / 10k redirects. Fixing was as easy as dropping style sheets and themes builds on redirects. It's kind of silly to build these on redirects, and obviously a flaw on our end performance wise, but I don't think there should be reason for the retained memory. Maybe this can help you guys narrow down the issue.

Fix:

export default ({clientStats}) => async (req, res) => {
  const store = await configureStore(req, res);

  if (!store) return null; // no store means redirect was already served

  const sheet = new ServerStyleSheet();
  const sheetsRegistry = new SheetsRegistry();
  const generateClassName = createGenerateClassName();
  const theme = createMuiTheme();

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 6, 2019

@kristfal Does it still happen in v4? What reference retains the objects in memory?

@pigchao
Copy link

pigchao commented Aug 28, 2019

I don't if we are the same problem, but it works for me after I add the props sheetsCache={null}

<StylesProvider sheetsManager={sheetsManager} sheetsRegistry={sheetsRegistry} sheetsCache={null}> <StaticRouter location={req.url} context={{}}> <App /> </StaticRouter> </StylesProvider>

@qhxin
Copy link

qhxin commented Jun 21, 2020

i have the same issue on browser #21528.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants