-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 from snapshot printing #5279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5279 +/- ##
=======================================
Coverage 60.98% 60.98%
=======================================
Files 203 203
Lines 6816 6816
Branches 3 4 +1
=======================================
Hits 4157 4157
Misses 2658 2658
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. thank you so much!
This is great work, thanks so much for diving in and triaging this. cc @mjesun the detectLeaks flag was helpful!! |
I just published 22.0.6 which includes this fix. cc @gaearon for React. |
Yeah @mjesun the detectLeaks flag did all the work here |
@rickhanlonii thanks! |
@rickhanlonii thanks a lot for that! I'm really happy to see the You rock! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This PR fixes a memory leak introduced in #5020
Fixes #5234
Fixes #5239
Fixes #5157
Details
I initially confirmed that
--detectLeaks
is broken for all tests. The next step was to figure out if this was a bug in the way we detect leaks or if it's a legit leak. Using this repo, I walked the commits backward to find the last passing test.That lead me to this commit. Nothing there is related to the leak detection, so I figured it must be a legit leak. From there I commented out lines until I could prove which line would fix the tests, then inspected the line.
It looks like we were keeping around references to the whole snapshot state. The fix is to remove the dependency using Array.from.
I'm not entirely sure how this is failing the environment leak detection, maybe the Snapshot State somehow keeps a reference to the environment?
Test plan
There are a couple of ways to test/demonstrate this.
1. via
--detectLeaks
Considering this test repo, running
yarn test
from master will fail:With this change they pass:
2. via React
This fix also fixes the memory issue @gaearon found in React for this command:
yarn test --coverage --runInBand --logHeapUsage
Result of Jest master (OOM at 1.5GB):
With this fix (capped at 500MB):