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

fix: memory leak caused by how marked library custom renderer being used #30

Merged

Conversation

maxbrereton
Copy link
Contributor

@maxbrereton maxbrereton commented Nov 18, 2024

Hi,

We have been using react-email in production under fairly heavy load and have noticed a memory leak.

After some investigation I have narrowed it down to this repo and specifically how the underlying marked library is being used currently.

I found this in marked library documentation: "Be careful: marked.use(...) should not be used in a loop or function. It should only be used directly after new Marked is created or marked is imported." (here: https://marked.js.org/using_advanced).

And the above warning from the marked documentation seems to be violated here in this repo: https://github.com/codeskills-dev/md-to-react-email/blob/master/src/parser.ts#L13

I have updated the code to use an alternative way of calling marked with a custom renderer instance.

Here are some memory snapshots from a small test I ran (using clinic.js to analyze memory):
Before fix:
Screenshot 2024-11-18 at 15 47 35

After fix:
Screenshot 2024-11-18 at 15 47 42

You can see that the memory profile is flat after the changes have been made (before it would eventually just crash with OOM error).

I also took this screenshot from running our production code locally and load testing the application and you can see that it retains the marked.js instances and does not release them. Eventually these build up and cause the OOM errors:
Screenshot 2024-11-12 at 14 26 29

I am not sure if more evidence is needed - I can push up my test case if you want? It was basically just calling parseMarkdownToJSX inside a nested loop thousands of times, similar to this (I used longer markdown example):

const { parseMarkdownToJSX } = require("./parseMarkdownToJSX");

const markdown = '## Lists'

async function testParse() {
  for (let i = 0; i < 800; i += 1) {
    for(let j = 0; j < 1000; j += 1) {
      parseMarkdownToJSX({ markdown });
    }
    await new Promise(r => setTimeout(r, 200));
    console.log('processed 1000, overall counter -> ', i);
  }
}

testParse()
  .then(() => {
    console.log('done');
  })
  .catch(err => {
    console.error('An error occurred:', err);
  });

Feel free to update my changes to modify what I have done (I don't have a lot of context on this codebase tbh). When I ran the tests locally all but one pass, and it fails snapshot test on quotes being encoded. However, this fails for me on master as well, so not too sure and might need advice on this.

@gabrielmfern gabrielmfern changed the title Fix memory leak caused by how marked library custom renderer being used fix: memory leak caused by how marked library custom renderer being used Nov 21, 2024
@gabrielmfern gabrielmfern merged commit 6762361 into codeskills-dev:master Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants