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

Add resetRetrieveHandlers to avoid memory leak #215

Merged
merged 1 commit into from
May 13, 2018
Merged

Add resetRetrieveHandlers to avoid memory leak #215

merged 1 commit into from
May 13, 2018

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Apr 30, 2018

I haven't added docs as I don't think this is the correct way to do this...

Background:

Jest has a CLI flag called --detectLeaks which tries to sniff out memory leaks in your test suite.
We discovered today that Jest itself seems to be leaking, and bisected it to jestjs/jest@3341d16. After fiddling a bit with it, I discovered that if I remove the whole retrieveSourceMap function from the options passed to install, it doesn't complain about a leak. Just making the function be empty and always return null still reported a leak.

The only way I was able to find that lets us keep using retrieveSourceMap but not leak was to add the function added in this PR.

It feels really hacky, so I'd love to find a way forward without needing this PR. 🙏

If you wanna mess around with fixes, to test in Jest just clone the repo, run yarn (followed by yarn watch if you wanna make changes to Jest's code base and not just in node_modules) and run ./jest --detectLeaks diff --no-cache. The diff I applied in Jest's code base to call the function added in this PR is this:

diff --git i/packages/jest-jasmine2/src/index.js w/packages/jest-jasmine2/src/index.js
index 9f144d34..e872c22f 100644
--- i/packages/jest-jasmine2/src/index.js
+++ w/packages/jest-jasmine2/src/index.js
@@ -161,9 +161,14 @@ async function jasmine2(
 
   runtime.requireModule(testPath);
   await env.execute();
-  return reporter
-    .getResults()
-    .then(results => addSnapshotData(results, snapshotState));
+
+  try {
+    const results = await reporter.getResults();
+
+    return addSnapshotData(results, snapshotState);
+  } finally {
+    sourcemapSupport.resetRetrieveHandlers();
+  }
 }
 
 const addSnapshotData = (results, snapshotState) => {

@SimenB
Copy link
Contributor Author

SimenB commented Apr 30, 2018

Side note: if #209 is accepted, I can do what I want without this new function being exposed by adding this diff:

diff --git i/packages/jest-jasmine2/src/index.js w/packages/jest-jasmine2/src/index.js
index 9f144d34..f0aca077 100644
--- i/packages/jest-jasmine2/src/index.js
+++ w/packages/jest-jasmine2/src/index.js
@@ -161,9 +161,16 @@ async function jasmine2(
 
   runtime.requireModule(testPath);
   await env.execute();
-  return reporter
-    .getResults()
-    .then(results => addSnapshotData(results, snapshotState));
+
+  try {
+    const results = await reporter.getResults();
+
+    return addSnapshotData(results, snapshotState);
+  } finally {
+    const sourcemapData = global[Symbol.for('source-map-support/sharedData')];
+
+    sourcemapData.retrieveMapHandlers.shift();
+  }
 }
 
 const addSnapshotData = (results, snapshotState) => {

Still super hacky (and really coupled to this lib using unshift and not push), so I'd really like to see a better solution for cleaning up after ourselves.

@SimenB
Copy link
Contributor Author

SimenB commented May 12, 2018

@LinusU thoughts on this? I basically need a way to uninstall the module

@LinusU
Copy link
Collaborator

LinusU commented May 13, 2018

Hmm, I see the problem and I think that this is a good enough fix for now 👍

Would be nice to look over the whole installation/uninstallation part, there are a few other issues open that are somewhat related...

Anyhow, let's release this for now 🚀

@LinusU LinusU merged commit be96fd9 into evanw:master May 13, 2018
@LinusU
Copy link
Collaborator

LinusU commented May 13, 2018

Published as 0.5.6 🥇

Thanks for the help!

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