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

createGlobalStyle creates memory leak in Next.js #3022

Closed
robbue opened this issue Feb 14, 2020 · 43 comments
Closed

createGlobalStyle creates memory leak in Next.js #3022

robbue opened this issue Feb 14, 2020 · 43 comments
Labels
needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved

Comments

@robbue
Copy link

robbue commented Feb 14, 2020

Environment

Binaries:

  • Node: 13.7.0 - ~/.nvm/versions/node/v13.7.0/bin/node
  • Yarn: 1.21.1 - /usr/local/bin/yarn
  • npm: 6.13.6 - ~/.nvm/versions/node/v13.7.0/bin/npm

npmPackages:

  • babel-plugin-styled-components: ^1.10.7 => 1.10.6
  • styled-components: ^5.0.1 => 5.0.1

Problem

We are using styled-components and Next.js (7.2.1) and we have experienced memory leaks in our app for some time, so I started to investigate it, in the process I found other issues here about:

  • using the css helper when composing styling partials to be interpolated into a styled component
  • not to use @import inside createGlobalStyle
  • not dynamically create components

So I went through the whole app making sure to avoid these. I deployed a new version but the memory leak was still present. As seen on the graph below:
Skjermbilde 2020-02-14 19 49 28

After inspecting the node server I found out it was createGlobalStyle in our _app.js (Next.js file) that caused the memory to rise. In the createGlobalStyle we used fontFace/normalize from polished), and around 50 other lines of reset css (nothing special).

Removing createGlobalStyle solved the memory leak (or at least reduced to a minimum).
See the graph below:
Skjermbilde 2020-02-14 19 50 55

Did we use createGlobalStylewrong or do someone have some insight to this?

@boyuan459
Copy link

in our next.js project, we can only use one createGlobalStyle, other global styles is replaced with next.js <style global jsx>

@PaulACoroneos
Copy link

Yes we are having the same issue in our Next app as well. Only difference is that we are using latest Next version.

@PaulACoroneos
Copy link

We confirmed the rollback resolved the issue. Our current style file is just a copy of normalize css and reset css with some font-face definitions. Something like

export const AppGlobalStyle = createGlobalStyle`
${reset}
${normalize}

@font-face {
...
`;

Where reset and normalize are something like

const normalize = html { line-height: 1.15; } ....

@PaulACoroneos
Copy link

One thing I noticed today. We have a block comment in our globalstyle. I wonder if this is possibly related to #3018

@robbue
Copy link
Author

robbue commented Feb 29, 2020

By removing createGlobalStyle the memory leak dropped significantly, but we still experience a leak (after 02/14). It went from 7 to 43 % memory utilization over 2 weeks, and we can live with that for now. But will try to investigate further.

Skjermbilde 2020-02-29 11 59 25

@ghengeveld
Copy link

ghengeveld commented Mar 1, 2020

We are running StoryShots over our 2000+ stories and after upgrading to Styled Components v5 our tests are timing out. I've been able to bring down the test time drastically by stripping our global styles (with createGlobalStyle) down to just a few simple css rules. At one point adding a single rule on an element selector increased test time from 12 minutes to 17 minutes. Adding more increased it to 55 minutes, and that wasn't even our complete set of global styles. Before upgrading, our tests took only 3 minutes or so, with the complete set of global styles from @storybook/design-system.

@jamosonic
Copy link

jamosonic commented Mar 3, 2020

We just had to revert a PR that added our first call to createGlobalStyle due to a memory leak.

image

Using [email protected] with a custom SSR set up (not NextJS).

The contents of the css in createGlobalStyle was pretty gnarly, an old sass based CSS reset - copied somewhat verbatim - comments and all.

@PaulACoroneos
Copy link

@JoshAmos Yes this is exactly what happened to us.

@quantizor
Copy link
Contributor

@ghengeveld are you still seeing that with 5.0.1? 5.0.0 had a subtle bug in it relating to SSR that could cause some weird lag.

We also have some unreleased changes in styled-components@test if y'all could give it a look.

@wwwebman
Copy link

wwwebman commented Mar 6, 2020

@probablyup, the issue is still there :(

Packages versions:
"styled-components": "^5.0.1"
"next": "9.2.2"

One of our global styles:

import { createGlobalStyle } from 'styled-components';

const AppGlobalStyles = createGlobalStyle`
  #__next {
     display: flex;
     flex-direction: column;
     height: 100vh;
  }
`;

AppGlobalStyles.displayName = 'AppGlobalStyles';

export default AppGlobalStyles;

Memory usage with createGlobalStyle ^^:
Screenshot from 2020-03-06 14-39-07

Memory usage without createGlobalStyle:
Screenshot from 2020-03-06 14-40-19

For sure we have some other small memory leaks, but createGlobalStyle produces the biggest one. This is how our memory usage looks using createGlobalStyle:

Screenshot from 2020-03-06 14-35-34

Constructor:
#__next{display:-webkit-box;display:-webkit-flex;display:-ms-flexbox;display:flex;-webkit-flex-direction:column;-ms-flex-direction:column;flex-direction:column;height:100vh;/*\|*/}:

Screenshot from 2020-03-06 14-46-53

The object referred to the constructor ^^:
Screenshot from 2020-03-06 14-47-45

As you can see the memory is being actively spammed on each page render by CSS string produced by createGlobalStyle output. This is a huge issue. Possible solutions:

  • Downgrade as mentioned by @ghengeveld
  • import '../global.css'
  • Fix createGlobalStyle ASAP

@GLObus303
Copy link

GLObus303 commented Mar 22, 2020

We were also forced to downgrade to 4.4.3. This is our memory consumption before and after styled-components v5, I am sure you can pinpoint the points where the version changed :)).

image

@alexparish
Copy link

alexparish commented Apr 12, 2020

I am also able to verify this. I have been suffering from a memory leak in my Next.js app and after plenty of searching came across this issue which appears to be the cause of my problems.

I am using createGlobalStyle and styled-normalize in _app.js and am able to see the global style strings building up in a Chrome DevTools Heap Snapshot:

Screen Shot 2020-04-12 at 15 44 05

Package versions:
"styled-components": "5.0.1" or "5.1.0"
"next": "9.3.1"

Downgrading styled-components to 4.4.1 resolved the issue.

You can see my _app.js here: https://gist.github.com/alexparish/ad63a7822884be77e16a093bfb5fd992

I'd be happy to create a repo if the maintainers would find it useful in reproducing the issue?

ryanjwessel added a commit to ryanjwessel/ryanjwessel.com that referenced this issue Apr 28, 2020
@alexparish
Copy link

Has anyone been able to resolve this?

I wonder if the issue is specifically with Next.js rather than styled-components. For example would any large string located in _app.js cause a memory leak?

@peeter-tomberg
Copy link

This is not related to Next.js. We're experiencing the same problems with a custom built React SSR implementation. Downgrading to version 4 solved the issue.

Seeing the same picture in my debugger as #3022 (comment)

@gregorskii
Copy link

gregorskii commented Jul 31, 2020

We ran into a similar issue where stylis trim function was causing a mem leak. Downgrading to 4.4.0, validating on server.

Screen Shot 2020-07-31 at 5 15 08 PM

EDIT: Confirmed.

EDIT 2: retracting confirmed, the memory is still rising on our server, but slightly slower.

EDIT 3: A heap snapshot:

Screen Shot 2020-07-31 at 6 43 47 PM

EDIT 4: mistook the last one, it was still on SC 5.0 due to one of our component libraries.

This is the memory snapshot from 4.4.0:

Screen Shot 2020-07-31 at 7 06 03 PM

Yep works fine when deployed:

Screen Shot 2020-07-31 at 7 40 58 PM

@Oikio
Copy link

Oikio commented Aug 3, 2020

Same problem on SSR with Next.js and Styled-components v5.1.1. Could not downgrade, so workaround was just not to use createGlobalStyle, which causes it.

@alexparish
Copy link

Same problem on SSR with Next.js and Styled-components v5.1.1. Could not downgrade, so workaround was just not to use createGlobalStyle, which causes it.

Out of interest what was your workaround? An alternative approach to using createGlobalStyle?

@Oikio
Copy link

Oikio commented Aug 3, 2020

Just a string in style tag, put in head:

export const GlobalStyles: any = () => (
  <style
    dangerouslySetInnerHTML={{
      __html: `
 ... styles ...
`
    }}
  />
)

Using Typescript, so had to cast an any type to use this component as before

UPD: One can also use just a .css file (in case of Next, which supports them out of the box), but we have variables there, so was not an option in my case.

@quantizor
Copy link
Contributor

Need a reproduction

@kitten kitten added the needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved label Aug 30, 2020
@gregorskii
Copy link

I think it's going to be quite hard to reproduce without access to one of our hosting environments. Can you check my heap dump photo above? That could elude to the issue.

@Oikio
Copy link

Oikio commented Aug 31, 2020

I have tried to create a bare bones (no frameworks) reproduction case, but I could not reproduce memory leak. Maybe I'm doing something wrong, so someone can take a look and update this repo: https://github.com/Oikio/styled-components-memory-leak-reproduction.

On our production we have Nextjs and lots of things happening of course, but I could see allocation of strings from createGlobalStyle growing. If I will find time later, I will try to create reproduction with Nextjs.

@ellioseven
Copy link

We ran into this issue on production. Like most others indicated, this was due to createGlobalStyle creating many strings on each page visit. Like others, we had to revert to v4.

Versions:

Will update when I have a reproduction available.

@Oikio
Copy link

Oikio commented Oct 21, 2020

I'm not common with the code base, but investigating it further led to Sheet.js, this particular line:

// line 69
  allocateGSInstance(id: string) {
    return (this.gs[id] = (this.gs[id] || 0) + 1);
  }

New style sheets are allocated later with this unique number in const styleSheet = useStyleSheet();, polluting the names map entry:
image

Why this happening and what is the reasoning behind incrementing in allocateGSInstance I don't know. Will investigate further, but it would be great if someone with better styled-components project knowledge could take a look at it.

@quantizor
Copy link
Contributor

@Oikio what does your global style look like, are there a lot of dynamic interpolations?

@quantizor
Copy link
Contributor

this.gs should be isolated to the ServerStyleSheet created on each SSR pass (it is not meant to be reused), so it's odd that it would have unbound growth like that unless you have a ton of global styles being injected all over the place

@Oikio
Copy link

Oikio commented Oct 21, 2020

@probablyup only 3 styles, they all look similar, not much happening there, I've even tried to remove all code from them, so that it would just a string passed to createGlobalStyle, but result was the same, here is an example of styles I have:

import { createGlobalStyle } from 'styled-components'
import { getColor } from './tokenGetter'

export const GlobalStyleReset = createGlobalStyle`
html {
  -webkit-text-size-adjust: 100%;
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
  -webkit-tap-highlight-color: transparent;
}

body {
  margin: 0;
  line-height: 1;
  font-family: sans-serif;
  background-color: ${getColor('background')};
}

iframe {
  border: 0;
}

ul,
ol {
  margin-top: 0;
  margin-bottom: 0;
  padding-left: 0;
}

li {
  display: block;
}

blockquote {
  margin: 0;
  padding: 0;
}

h1,
h2,
h3,
h4,
h5,
h6,
p {
  margin: 0;
  padding: 0;
  border: 0;
  font-size: inherit;
  font-weight: inherit;
  color: inherit;
  background: none;
}

strong {
  font-weight: bold;
}

img {
  border: 0;
  max-width: 100%;
  height: auto;
  vertical-align: middle;
}

a {
  color: inherit;
}

button {
  border: 0;
  margin: 0;
  padding: 0;
  text-align: center;
  text-transform: inherit;
  font: inherit;
  -webkit-font-smoothing: inherit;
  letter-spacing: inherit;
  background: none;
  cursor: pointer;
  overflow: visible;
}

::-moz-focus-inner {
  border: 0;
  padding: 0;
}

body {
  -webkit-font-smoothing: subpixel-antialiased;
}

body.noscroll {
  overflow: hidden;
  padding-right: 15px;
}

* {
  box-sizing: border-box;
  outline: 0;
}

img {
  display: block;
}

*:before,
*:after {
  box-sizing: inherit;
}  
`

@Oikio
Copy link

Oikio commented Oct 22, 2020

Still not much progress in finding our why this.names field is reused, though ServerStylesheet is recreated on every request. We are using integration from examples of Next.js:

  static async getInitialProps(ctx: DocumentContext) {
    const sheet = new ServerStyleSheet()
    const originalRenderPage = ctx.renderPage

    try {
      ctx.renderPage = () =>
        originalRenderPage({
          enhanceApp: App => props => sheet.collectStyles(<App {...props} />)
        })

      const initialProps = await Document.getInitialProps(ctx)
      return {
        ...initialProps,
        styles: (
          <>
            {initialProps.styles}
            {sheet.getStyleElement()}
          </>
        )
      }
    } finally {
      sheet.seal()
    }
  }

It looks pretty straightforward and I wanted to blame Next.js first, until I saw that version 4 was fine and also there is a comment from peeter-tomberg above, having same issue with non Next.js project.
Will continue to investigate, help appreciated.

BTW, @peeter-tomberg if you can share how styled-components are integrated in your app without Next, it would great for invsetigating the issue.

@Oikio
Copy link

Oikio commented Oct 23, 2020

Further update, id argument in Stylesheet.clearNames is always forward by 1. Like if id is sc-global-xxx5, then during this call this.names would contain only sc-global-xxx4 and below. Though there is always one call during render with id=1.

I'm still struggling to understand the flow of the calls to make clear picture of why this is happening.

@Oikio
Copy link

Oikio commented Oct 26, 2020

Allocation always increments id here:

  allocateGSInstance(id: string) {
    return (this.gs[id] = (this.gs[id] || 0) + 1);
  }

But previous id for createGlobalStyle is never cleared, here is the flow of the renderStyles:

...
    if (__SERVER__) {
      renderStyles(instance, props, styleSheet, theme, stylis); // instance here is "current id"
    } else {
...
->
...
globalStyle.renderStyles(instance, STATIC_EXECUTION_CONTEXT, styleSheet, stylis);
...
->
...
  renderStyles(
    instance: number,
    executionContext: Object,
    styleSheet: StyleSheet,
    stylis: Stringifier
  ) {
    if (instance > 2) StyleSheet.registerId(this.componentId + instance);

    // NOTE: Remove old styles, then inject the new ones
    this.removeStyles(instance, styleSheet); // and it's called here with current id, this.removeStyles() is never called for previous id.
    this.createStyles(instance, executionContext, styleSheet, stylis);
  }
..

At least it looks like the issue. I'm not sure where to fix it.
Why does allocateGSInstance increments numerical part of id, isn't id hash unique?
Where should removeStyles() be called for previous allocated styles?

@quantizor
Copy link
Contributor

Why does allocateGSInstance increments numerical part of id, isn't id hash unique?

You can mount multiple instances of a global style in your application and we use ID to track the individual invocations so when they're unmounted they're removed in the proper order.

@Oikio
Copy link

Oikio commented Oct 26, 2020

Hm, I don't see removing happening here for previous ids on new calls, where can I look at it?

@Oikio
Copy link

Oikio commented Oct 27, 2020

Now I'm getting it a bit. Looks like this bug is a bit more than just styled-components. It's not reproducible with Next unless one adds apollo-client for graphql. Looks like in our app getDataFromTree method is leaving references somehow. Will have to investigate it further.

Reporters, do you use getDataFromTree method in your applications with this leak?

@wwwebman
Copy link

wwwebman commented Oct 27, 2020

@Oikio Yes, but how is it related to the problem? The issue happens when we migrate from v4 to v5 using getDataFromTree in both cases...

@Oikio
Copy link

Oikio commented Oct 27, 2020

That's what I will try to figure out. At least hope so, I'm already running out of time for the ticket I'm on.

@alexparish
Copy link

@Oikio Yes, I am also using apollo-client and facing this issue.

@Oikio
Copy link

Oikio commented Oct 28, 2020

Looks like it's happening because when using getDataFromTree, it's internal call to React.renderToStaticMarkup not using context StyleSheet, provided by

export function useStyleSheet(): StyleSheet {
  return useContext(StyleSheetContext) || masterSheet;
}

Instead masterSheet is reused. That leads to polluting of masterSheet.name map via allocateGSInstance.
Figuring out why context StyleSheet is not available for getDataFromTree, have no idea how it is provided in the first place, any suggestion where to look at?

@quantizor
Copy link
Contributor

Good question for the Apollo team I guess 🤷‍♂️ sheet.collectStyles wraps the passed JSX tree in a StyleSheetManager (our context provider) with the server stylesheet instance specified.

@quantizor
Copy link
Contributor

It could be something dumb like mismatched React versions in the tree leading to mismatched context symbols or something like that. Have you tried forcing a consistent version of react and react-dom via yarn resolutions?

@Oikio
Copy link

Oikio commented Oct 28, 2020

Version mismatch leads to lots of errors in the runtime, so it shouldn't be it, but I will check it out tomorrow and will take a closer look how context should be provided. getDataFromTree is quite straightforward, so shouldn't take long.

@Oikio
Copy link

Oikio commented Oct 29, 2020

So I'm giving up on properly solving this issue, I can't figure out how to pass context in my particular case with getDataFromTree(<Tree />, ctx), because I'm not sure where to even take it. But issue is definitely as I described here. I still wonder why masterSheet exists, because it's singleton which is never cleaned up, at least from my understanding.

Nevertheless I have found a workaround to mitigate this issue:

            await getDataFromTree(
              // workaround for memory leak, see https://github.com/styled-components/styled-components/issues/3022
              <StyleSheetManager sheet={new ServerStyleSheet().instance}>
                <AppTree {...props} />
              </StyleSheetManager>
            )

Just wrapping it with StyleManager will make it use new StyleSheet every time. Not sure if there will be any harmful side effects here.

@quantizor
Copy link
Contributor

@Oikio that's fine to do, it's essentially what sheet.collectStyles does anyway

@Oikio
Copy link

Oikio commented Oct 29, 2020

By the way this issue should go away with Suspense coming to React SSR and Apollo, not that it's soon, but at least if my solution works for others with same problem - we should be fine.

@kitten
Copy link
Member

kitten commented Aug 9, 2021

Closing, since I don't think there is a proper way to resolve this with the current implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info ✋ An issue that needs triaging and reproducible examples or more information to be resolved
Projects
None yet
Development

No branches or pull requests