BuilderIO / builder

Drag and drop headless CMS for React, Vue, Svelte, Qwik, and more
https://builder.io
MIT License
6.77k stars 840 forks source link

Critical memory issue since 1.0.21 #3242

Closed MaximeGratens closed 2 weeks ago

MaximeGratens commented 3 weeks ago

Describe the bug Hello, Since 1.0.21 and using the ini To Reproduce Steps to reproduce the behavior:

  1. Install sdk react 2 with remix
  2. Use the initiate node sdk inside loader
  3. Deploy in a production server and monitore

Expected behavior A clear and concise description of what you expected to happen. The memory or cpu of the server will increase until it will crash

Screenshots If applicable, add screenshots to help explain your problem. image

Additional context Add any other context about the problem here. We deploy the sdk 1.0.23 without the initiate node and you can see in the screen that the memory issue is fixed

mikehuebner commented 2 weeks ago

Seeing this as well on 1.0.23, it just crashed our beanstalk application with a bunch of errors, removing the initializeNodeRuntime causes the below error. However, just removing the initializeNodeRuntime fixes the issue.

[Builder.io]:  Failed code evaluation: [Builder.io]: could not import `isolated-vm` module for safe script execution on Node server.      In certain Node environments, the SDK requires additional initialization steps. This can be achieved by   importing and calling `initializeNodeRuntime()` from "@builder.io/sdk-react/node/init". This must be done in   a server-only execution path within your application.   Please see the documentation for more information: https://builder.io/c/docs/integration-tips#enabling-data-bindings-in-node-environments    { code: 'state.posts' }
MaximeGratens commented 2 weeks ago

Seeing this as well on 1.0.23, it just crashed our beanstalk application with a bunch of errors, removing the initializeNodeRuntime causes the below error. However, just removing the initializeNodeRuntime fixes the issue.

[Builder.io]:  Failed code evaluation: [Builder.io]: could not import `isolated-vm` module for safe script execution on Node server.      In certain Node environments, the SDK requires additional initialization steps. This can be achieved by   importing and calling `initializeNodeRuntime()` from "@builder.io/sdk-react/node/init". This must be done in   a server-only execution path within your application.   Please see the documentation for more information: https://builder.io/c/docs/integration-tips#enabling-data-bindings-in-node-environments    { code: 'state.posts' }

If you remove the initializeNodeRuntime and use symbols, your site's performance will be degraded.

samijaber commented 2 weeks ago

We just published some perf improvements: reusing an Isolate VM context across all data binding evaluations. This should reduce memory usage during SSR.

Available at 1.0.25, does not require any change besides updating the version.

https://github.com/BuilderIO/builder/releases/tag/%40builder.io%2Fsdk-react%401.0.25

samijaber commented 2 weeks ago

@MaximeGratens If you can try this new version and let me know how it fares performance-wise, that would be great.

MaximeGratens commented 2 weeks ago

I get this in local and production : TypeError: Cannot destructure property 'ivmIsolateOptions' of 'undefined' as it is undefined.

kaceycleveland commented 2 weeks ago

As a work around for now @MaximeGratens , can pass an empty object to initializeNodeRuntime:

initializeNodeRuntime({})
kevintruby commented 2 weeks ago

Wow, what timing! I was going to open a similar issue for the Vue SDK, but now it might make more sense to comment here instead.

I've been delaying a major upgrade from the @builder.io/sdk-vue package's 10/2023 0.7.0 release to the latest, because our E2E tests were experiencing periods of downtime when testing a staging site. Upon investigation it seemed the introduction of isolated-vm at version 0.7.1 or 0.7.2 (could never get 0.7.1 to work) alone resulted in a major memory leak.

It took some time to dig into what part of our content caused this, because some pages behaved fine. In our case, any version that included isolated-vm prior to this 1.0.25 update struggled with pages that included symbols that loop through a List input's contents and render images. Our use case was a symbol that could generate a row of CTAs for our e-commerce site.

I recreated a very narrow version of the issue with a quick Nuxt repo for SSR plus a lightweight Docker container that could be used to watch memory use over time, like so:

Screenshot 2024-04-29 at 5 47 33 PM

Will keep that around just in case, but I updated to 1.0.25 and the narrow reproduction no longer spikes. Applying this to my actual site's environment also appears to behave as expected. I'll know for sure after automated tests run on this build, but I feel confident after examining the local Docker container.

Figured this could be anecdotal evidence of the fix for other SSR environments 🤞

MaximeGratens commented 2 weeks ago

We're in production with version 1.0.25 and the initializeNodeRuntime({}) function;

So far, we haven't seen any crashes or memory limits (we're testing on a multiple instance with 512mb of ram).

The main page of our product takes between 1 and 2 seconds to load, which isn't very SEO-friendly and isn't what you're selling on your website. We can't use a cache because these pages are loaded from google or facebook ads with different query parameters each time. image

We always have very slow pages and it's the same thing as @kevintruby, it's when we try to loop.

You have this inside issiue #3240

MaximeGratens commented 2 weeks ago

image Here is the last 6 hours with the the 1.0.25 in production.

As you can see we had a crash few minutes ago.

Also, the memory graph is completely jagged.

samijaber commented 2 weeks ago

@kevintruby Thanks for sharing all of these insights. By reusing Isolate contexts across all data-binding evaluations, the latest change should get rid of memory leaks. Do let me know if there are any persistent issues when you deploy your Vue SDK test to production.

@MaximeGratens Thanks for testing the latest version. It sounds like this issue may be specific to the builder feature of iterating over an array of dynamically fetched data? I will respond in your other issue.

samijaber commented 2 weeks ago

I am closing this issue as the specific memory leak caused by isolated-vm has been resolved by reusing a single IsolatedVM Context per render.

The unrelated performance issue tied to repeated blocks on data bindings can be tracked here https://github.com/BuilderIO/builder/issues/3240

samijaber commented 2 weeks ago

Additionally: I patched initializeNodeRuntime so that you do not need to provide an empty object as a parameter.

See v1.0.26: https://github.com/BuilderIO/builder/blob/main/packages/sdks/output/react/CHANGELOG.md#1026

MaximeGratens commented 2 weeks ago

@samijaber I don't think the problem has been really solved.

2 grafs of 2 differents projects using builder and remix.

image image

Both server are crashing after sometimes, we are using 1.0.26. This is the last 24 hours of our production server.

khiemtong commented 2 weeks ago

As another data point, I can confirm that this fix resolved our memory issues with NextJS@14.2.3 and @builder.io/sdk-react@1.0.25

samijaber commented 2 weeks ago

Thanks folks. @MaximeGratens we are actively investigating the other sources of memory issues that you are facing and will respond to them in #3240