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

[Bug Report][3.0.0] Memory leak during ssr in nuxt #16156

Closed
tobiasklemp opened this issue Nov 24, 2022 · 14 comments
Closed

[Bug Report][3.0.0] Memory leak during ssr in nuxt #16156

tobiasklemp opened this issue Nov 24, 2022 · 14 comments
Assignees
Labels
E: theme Theme composable NUXT NUXT related issues T: bug Functionality that does not work as intended/expected
Milestone

Comments

@tobiasklemp
Copy link
Contributor

Environment

Vuetify Version: 3.0.0
Vue Version: 3.2.45
Browsers: Chrome 107.0.0.0
OS: Mac OS 10.15.7

Steps to reproduce

Environment

  • Operating System: Darwin
  • Node Version: v16.13.2
  • Nuxt Version: 3.0.0
  • Nitro Version: 1.0.0
  • Package Manager: yarn@1.22.19
  • Builder: vite
  • User Config: css, build, vite
  • Runtime Modules: -
  • Build Modules: -

Reproduction

Clone this minimal starter project:
https://github.com/CodyBontecou/nuxt3-and-vuetify

do a

yarn 
yarn build

Then run the build with

node --expose-gc --inspect .output/server/index.mjs

Open the devtools and watch the memory tab.

I used k6 to run a small load test against my project:

import http from 'k6/http'
import { sleep } from 'k6'

export const options = {
  stages: [
    { duration: '20s', target: 100 }, // simulate ramp-up of traffic from 1 to 100 users over 5 minutes.
    { duration: '20s', target: 100 }, // stay at 100 users for 10 minutes
    { duration: '20s', target: 0 }, // ramp-down to 0 users
  ],
}

const BASE_URL = 'http://localhost:3000'

export default () => {
  http.get(`${BASE_URL}`)
  sleep(1)
}

Describe the bug

After running the test, about 160 mb of ram get stuck, indicating that something is leaking. After some investigation it appears to be related with the usage of the head composable in combination with a computed value outside of a component context.

This is the relevant code, which is causing the leak:

// node_modules/vuetify/lib/composables/theme.mjs
function install(app) {
    const head = app._context.provides.usehead;
    if (head) {
      head.addHeadObjs(computed(() => {
        const style = {
          children: styles.value,
          type: 'text/css',
          id: 'vuetify-theme-stylesheet'
        };
        if (parsedOptions.cspNonce) style.nonce = parsedOptions.cspNonce;
        return {
          style: [style]
        };
      }));
      if (IN_BROWSER) {
        watchEffect(() => head.updateDOM());
      }
    } else {
      let styleEl = IN_BROWSER ? document.getElementById('vuetify-theme-stylesheet') : null;
      watch(styles, updateStyles, {
        immediate: true
      });
      function updateStyles() {
        if (parsedOptions.isDisabled) return;
        if (typeof document !== 'undefined' && !styleEl) {
          const el = document.createElement('style');
          el.type = 'text/css';
          el.id = 'vuetify-theme-stylesheet';
          if (parsedOptions.cspNonce) el.setAttribute('nonce', parsedOptions.cspNonce);
          styleEl = el;
          document.head.appendChild(styleEl);
        }
        if (styleEl) styleEl.innerHTML = styles.value;
      }
    }
  }

I found this issue which also indicates that this is a problem, but I don't know how i would be able to use vuetify during ssr.

Expected Behavior

Run without leaking memory.

Actual Behavior

It is leaking a quite significant amount of memory.

Reproduction Link

https://github.com/CodyBontecou/nuxt3-and-vuetify

@Denoder
Copy link

Denoder commented Nov 28, 2022

@tobiasklemp can you try manually going into theme.mjs and changing:

head.addHeadObjs(computed(() => {
  const style = {
    children: styles.value,
    type: 'text/css',
    id: 'vuetify-theme-stylesheet'
  };
  if (parsedOptions.cspNonce) style.nonce = parsedOptions.cspNonce;
  return {
    style: [style]
  };
}));
if (IN_BROWSER) {
  watchEffect(() => head.updateDOM());
}

to:

function updateStyles() {
  head.push(computed(() => {
    const style = {
      children: styles.value,
      type: 'text/css',
      id: 'vuetify-theme-stylesheet'
    };
    if (parsedOptions.cspNonce) style.nonce = parsedOptions.cspNonce;
    return {
      style: [style]
    };
  }))
}

if (IN_BROWSER) {
  watch(styles, updateStyles, {
    immediate: true
  });
}

and see if that stops the memory leaks?

@tobiasklemp
Copy link
Contributor Author

@teranode Yes, that stops the memory leak.

@johnleider johnleider added T: bug Functionality that does not work as intended/expected NUXT NUXT related issues E: theme Theme composable and removed S: triage labels Nov 29, 2022
@johnleider johnleider added this to the v3.0.x milestone Nov 29, 2022
@johnleider johnleider self-assigned this Nov 29, 2022
@KaelWD KaelWD modified the milestones: v3.0.x, v3.1.x Jan 6, 2023
@harlan-zw
Copy link

Just FYI the latest vueuse/head is fully reactive so you shouldn't need this

 if (IN_BROWSER) {
        watchEffect(() => head.updateDOM());
      }

@KaelWD
Copy link
Member

KaelWD commented Feb 20, 2023

@harlan-zw I don't think that would help this issue, it only runs in the browser. Is there any way to detect if the installed vueuse/head is already reactive?

@KaelWD KaelWD assigned KaelWD and unassigned johnleider Feb 20, 2023
@KaelWD
Copy link
Member

KaelWD commented Feb 20, 2023

I've had other problems with memory leaks from SSR, ended up doing this to fix it: vuetifyjs/vite-ssg@ff268ab

@harlan-zw
Copy link

harlan-zw commented Feb 20, 2023

@harlan-zw I don't think that would help this issue, it only runs in the browser. Is there any way to detect if the installed vueuse/head is already reactive?

Maybe related nuxt/nuxt#15095? I'd recommend swapping computed for a computer-getter either way.

Computed getter / fully reactive is supported since 0.8.0 (https://github.com/vueuse/head/releases/tag/v0.8.0), so should be safe to use.

No guarantees though 🙃

head.addHeadObjs({
  style: [
      {
          // you could probably just pass in styles directly as well
          children: () => styles.value,
          type: 'text/css',
          id: 'vuetify-theme-stylesheet'
         nonce: parsedOptions.cspNonce || false
      }
  ]
})      

@KaelWD
Copy link
Member

KaelWD commented Feb 20, 2023

This doesn't actually seem to be reactive anymore no matter what I do. 0.7.6 was fine, but with 1.0.26 none of these update:

head.push({
  style: [{
    children: styles,
    id: 'vuetify-theme-stylesheet',
    nonce: parsedOptions.cspNonce,
  }],
})

head.push({
  style: [{
    children: () => styles.value,
    id: 'vuetify-theme-stylesheet',
    nonce: parsedOptions.cspNonce,
  }],
})

head.push(() => ({
  style: [{
    children: styles.value,
    id: 'vuetify-theme-stylesheet',
    nonce: parsedOptions.cspNonce,
  }],
}))

head.push(computed(() => ({
  style: [{
    children: styles.value,
    id: 'vuetify-theme-stylesheet',
    nonce: parsedOptions.cspNonce,
  }],
})))

same with head.addHeadObjs instead and adding back watchEffect(() => head.updateDOM())

@KaelWD
Copy link
Member

KaelWD commented Feb 20, 2023

#16721 if you want to have a poke around

@tobiasklemp
Copy link
Contributor Author

Hey,
the recent changes didn't do the trick. I ran my above benchmark again using vuetify 3.1.3 with the same result and i discovered that

const parsedOptions = reactive(parseThemeOptions(options))
causes the leak. I'm not sure if these options need to be reactive, in my view these options are loaded once and i don't see how these would change after that, but maybe someone else has more insight to this.

When the parsed options are not reactive, the leak is gone for the most part.

Running my benchmark, it creates about 35K ssr requests:

  • with reactive options about 2.5gb gets stuck in ram
  • without the reactive about 60mb gets stuck in ram

I was not able to isolate the last small leak, but maybe you will be able to fix this big one.

@johnleider
Copy link
Member

causes the leak. I'm not sure if these options need to be reactive, in my view these options are loaded once and i don't see how these would change after that, but maybe someone else has more insight to this.

Maybe @nekosaur could provide some insight to the necessity for reactive.

@KaelWD
Copy link
Member

KaelWD commented Jul 10, 2023

#16721 didn't fix this, and I never claimed that it did

@KaelWD KaelWD reopened this Jul 10, 2023
@KaelWD KaelWD modified the milestones: v3.1.x, v3.3.x Jul 10, 2023
@KaelWD
Copy link
Member

KaelWD commented Jul 10, 2023

The only thing I could think of was being able to provide a reactive object, but that doesn't actually work because parseThemeOptions does a deep copy: Playground

@memic84
Copy link

memic84 commented Oct 6, 2023

The only thing I could think of was being able to provide a reactive object, but that doesn't actually work because parseThemeOptions does a deep copy: Playground

Is this solved?

@johnleider
Copy link
Member

We kindly ask users to not comment on closed/resolved issues. If you believe that this issue has not been correctly resolved, please create a new issue showing the regression or create a new discussion.

If you have any questions, please reach out to us in our Discord community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: theme Theme composable NUXT NUXT related issues T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

No branches or pull requests

6 participants