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

Memory leak with rw test web when run sequentially - CI machines #4569

Open
callingmedic911 opened this issue Feb 24, 2022 · 6 comments
Open

Comments

@callingmedic911
Copy link
Collaborator

#4208 had the changes to run web tests in parallel which reduced the frequency of leaks by running tests in parallel but in Github Action, where CPU is limited, the parallel execution is not always possible, which results in frequent crashes due to out of memory.
Here's an example repo: https://github.com/callingmedic911/rw-web-memory-leak with CI check which fails on the latest Node LTS (16.14). Heap usage is better with Node 16.10 but the leak is still visible and if we increase the number of tests + mocks it'll also crash with Node 16.10 or lower.

Example CI run: https://github.com/callingmedic911/rw-web-memory-leak/runs/5321129020

This leak is exacerbated due to jestjs/jest#11956 but since we're require-ing all of mock in the project for each unit test across all test files, it's causing massive heap usage:

beforeEach(async () => {
// Import the global mocks.
for (const m of project.mocks) {
require(m)
}

I think for the next steps, we can evaluate:

  1. (Context: Don't merge: Remove global mocks in web's jest setup to improve memory usage #4457 (comment))
    instead of importing all mocks for each test, if we can somehow require only required ones. The idea is to put the import statement for a mock file inside the corresponding cell, so even if we have nested cells, the corresponding mock file would be imported. A good place to validate this would be by modifying the "auto-mock" babel plugin to insert the import statement in cell.
  2. as last resort we can expose-gc and force gc after each test suite run.
@thedavidprice
Copy link
Contributor

This is excellent work @callingmedic911 Thank you!

@ortegoncarlos
Copy link

Hi @callingmedic911, I have been facing problems with memory heap on github actions, one of the solutions I found is importing the mocks only once under redwood/packages/testing/config/jest/web/jest.setup.js.

instead of:

beforeEach(async () => { 
   // Import the global mocks. 
   for (const m of project.mocks) { 
     require(m) 
   } 

You can do:

beforeAll(async () => {
  // Import the global mocks.
  for (const m of project.mocks) {
    require(m)
  }
  await startMSW('node')
})

beforeEach(async () => {
  setupRequestHandlers() // reset the handlers in each test.
})

Something else I found it's in my mac machine runs smoothly memory heap is stable, but on github actions ubuntu-latest consumes more memory 🤷🏽‍♂️, and finally NODE_OPTIONS: "--max_old_space_size=4096" is override by something in redwood cli.

if you run node --max_old_space_size=6144 node_modules/.bin/jest takes effect but if you run node --max_old_space_size=6144 node_modules/@redwoodjs/core/dist/bins/redwood.js test has no effect.

@razzeee
Copy link
Contributor

razzeee commented Aug 25, 2022

How were you able to test that against your codebase? npm link somehow or with a different approach?

@callingmedic911
Copy link
Collaborator Author

@ortegoncarlos thanks for sharing your finding in detail. I believe there's PR open recently to reduce bloat from mock #6281.
About the changing node/v8 options, I felt this may not be an ideal solution - and may have unintended side effects. I'm open to exploring all the ideas though.

@razzeee Using yarn rwfw from your project you can link your local RedwoodJS repo to your project and try things out. More details here: https://github.com/redwoodjs/redwood/blob/main/CONTRIBUTING.md#testing-the-framework-in-your-project

@razzeee
Copy link
Contributor

razzeee commented Aug 25, 2022

Thanks!

I'm not convinced, this is CI specific. I'm also having this locally (fedora) and I think my colleague also sees it on mac os.


This part is probably obsolete now, that you mention that PR, still doesn't hurt.

I took https://github.com/redwoodjs/redwood-tutorial and run it with this:

yarn rw test web --detectOpenHandles --logHeapUsage

Which returned:

 PASS   web  web/src/components/Article/Article.test.js (6.636 s, 392 MB heap size)
 PASS   web  web/src/components/ArticleCell/ArticleCell.test.js (503 MB heap size)
 PASS   web  web/src/layouts/BlogLayout/BlogLayout.test.js (706 MB heap size)
 PASS   web  web/src/components/ArticlesCell/ArticlesCell.test.js (918 MB heap size)
 PASS   web  web/src/pages/ContactPage/ContactPage.test.js (1123 MB heap size)
 PASS   web  web/src/pages/ArticlePage/ArticlePage.test.js (1323 MB heap size)
 PASS   web  web/src/pages/AboutPage/AboutPage.test.js (985 MB heap size)
 PASS   web  web/src/pages/HomePage/HomePage.test.js (1178 MB heap size)

So it's not enough to crash the process, but it still seems like way to much heap for those tests?

@ortegoncarlos
Copy link

Just to report, that after update with redwood 3.x the memory leaks went away. so this probably should be close

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

No branches or pull requests

5 participants