Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix(memory): reduce memory leaks in Jest JSDOM environment #1106

Merged
merged 1 commit into from
Jul 17, 2018

Conversation

FreeleX
Copy link
Contributor

@FreeleX FreeleX commented Jun 26, 2018

This is related to angular/angular#24048 (comment)

As JSDOM shares prototypes we have a leak after directly patching them.
The proposed solution is to wrap global functions with delegators and also their prototypes same way.
This way zone.js will patch only delegators making originals untouched and no memory leak therefore will hapen.

In order to reproduce the issue please refer to https://github.com/FreeleX/jest-angular-memory-leak-example and for more info to the angular/angular#24048

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@FreeleX FreeleX changed the title fix(memory leaks in jsdom): wrap objects for save prototype patching fix(memory leaks in jsdom): wrap objects for safe prototypes patching Jun 26, 2018
@JiaLiPassion
Copy link
Collaborator

@FreeleX , I am not quite understand why there is a leak, and based on the comment angular/angular#24048 (comment), you said the Function maybe patched multiple times, in zone.js current implementation, if a function is already patched, it will not be patched again, if it does, it will be a bug. So could you describe more about the reason of the leak? I will also check your reproduce repo, thank you.

@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 2, 2018

@JiaLiPassion Thank you for considering this issue further. I believe I have no strong proof of why this patch works, neither do I have a clear vision that it is an appropriate patch (and many tests failed, which I suppose not a good sign).
I'm just trying to find a way to dig deeper into this issue.

Seems I was wrong about patched multiple times functions (found protections in code against overpatching), sorry for that, but may be right about patching 'on_' properties.
I found a way to patch in more simple way (also Jest reports there are memory leaks, but heap stopped growing steadily so high). Maybe this will get us closer to the reason.

Tested on https://github.com/FreeleX/jest-angular-memory-leak-example repo:

Without patch:

> jest --runInBand --logHeapUsage

 PASS  __tests__/test24.spec.js (45 MB heap size)
 PASS  __tests__/test20.spec.js (46 MB heap size)
 PASS  __tests__/test8.spec.js (47 MB heap size)
 PASS  __tests__/test13.spec.js (53 MB heap size)
 PASS  __tests__/test3.spec.js (53 MB heap size)
 PASS  __tests__/test14.spec.js (54 MB heap size)
 PASS  __tests__/test11.spec.js (67 MB heap size)
 PASS  __tests__/test15.spec.js (67 MB heap size)
 PASS  __tests__/test22.spec.js (67 MB heap size)
 PASS  __tests__/test16.spec.js (68 MB heap size)
 PASS  __tests__/test2.spec.js (81 MB heap size)
 PASS  __tests__/test1.spec.js (81 MB heap size)
 PASS  __tests__/test21.spec.js (82 MB heap size)
 PASS  __tests__/test5.spec.js (83 MB heap size)
 PASS  __tests__/test6.spec.js (83 MB heap size)
 PASS  __tests__/test17.spec.js (96 MB heap size)
 PASS  __tests__/test9.spec.js (96 MB heap size)
 PASS  __tests__/test18.spec.js (96 MB heap size)
 PASS  __tests__/test25.spec.js (98 MB heap size)
 PASS  __tests__/test23.spec.js (110 MB heap size)
 PASS  __tests__/test10.spec.js (110 MB heap size)
 PASS  __tests__/test4.spec.js (110 MB heap size)
 PASS  __tests__/test7.spec.js (111 MB heap size)
 PASS  __tests__/test12.spec.js (123 MB heap size)
 PASS  __tests__/test19.spec.js (124 MB heap size)

Test Suites: 25 passed, 25 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        2.166s
Ran all test suites.

With patch:

> jest --runInBand --logHeapUsage

 PASS  __tests__/test24.spec.js (45 MB heap size)
 PASS  __tests__/test8.spec.js (45 MB heap size)
 PASS  __tests__/test22.spec.js (47 MB heap size)
 PASS  __tests__/test11.spec.js (49 MB heap size)
 PASS  __tests__/test20.spec.js (50 MB heap size)
 PASS  __tests__/test14.spec.js (51 MB heap size)
 PASS  __tests__/test18.spec.js (63 MB heap size)
 PASS  __tests__/test2.spec.js (63 MB heap size)
 PASS  __tests__/test13.spec.js (64 MB heap size)
 PASS  __tests__/test21.spec.js (65 MB heap size)
 PASS  __tests__/test19.spec.js (77 MB heap size)
 PASS  __tests__/test17.spec.js (77 MB heap size)
 PASS  __tests__/test1.spec.js (50 MB heap size)
 PASS  __tests__/test6.spec.js (50 MB heap size)
 PASS  __tests__/test9.spec.js (51 MB heap size)
 PASS  __tests__/test12.spec.js (64 MB heap size)
 PASS  __tests__/test10.spec.js (64 MB heap size)
 PASS  __tests__/test16.spec.js (64 MB heap size)
 PASS  __tests__/test23.spec.js (65 MB heap size)
 PASS  __tests__/test7.spec.js (78 MB heap size)
 PASS  __tests__/test15.spec.js (78 MB heap size)
 PASS  __tests__/test3.spec.js (49 MB heap size)
 PASS  __tests__/test5.spec.js (50 MB heap size)
 PASS  __tests__/test25.spec.js (50 MB heap size)
 PASS  __tests__/test4.spec.js (63 MB heap size)

Test Suites: 25 passed, 25 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        2.141s
Ran all test suites.

The patch is:

diff --git a/lib/common/utils.ts b/lib/common/utils.ts
index e7bbed3..e3d190a 100644
--- a/lib/common/utils.ts
+++ b/lib/common/utils.ts
@@ -239,7 +239,11 @@ export function patchProperty(obj: any, prop: string, prototype?: any) {
     return null;
   };
 
-  ObjectDefineProperty(obj, prop, desc);
+  if (!obj['__is__zone_prop_' + prop + '_set']) {
+    obj['__is__zone_prop_' + prop + '_set'] = true;
+
+    ObjectDefineProperty(obj, prop, desc);
+  }
 }
 
 export function patchOnProperties(obj: any, properties: string[]|null, prototype?: any) {

I've found out that Zone.__load_patch('on_property causes major leak and discovered this solution by going through:
Zone.__load_patch('on_property' ->
propertyDescriptorPatch ->
patchFilteredProperties(XMLHttpRequestEventTarget && XMLHttpRequestEventTarget.prototype ->
patchOnProperties ->
patchProperty

I hope this will help with further investigation of the issue in some way :)

Open to any suggestions. Thank you

@JiaLiPassion
Copy link
Collaborator

@FreeleX , thank you very much for the detailed researh, I will check it.

@JiaLiPassion
Copy link
Collaborator

@FreeleX , I run your sample, and it seems XMLHttpRequestEventTarget will be patched multiple times,
so I think the PR should be just as your suggested, modify patchProperty method, and check whether the property has been checked.

And to be consistant, please keep the property name here

+    obj['__is__zone_prop_' + prop + '_set'] = true;

to be

obj[zoneSymbol(`on` + prop + 'patched')] = true;

Thank you for the research, it really helped.

@FreeleX FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 07679eb to ef7116f Compare July 3, 2018 07:44
@FreeleX FreeleX changed the title fix(memory leaks in jsdom): wrap objects for safe prototypes patching fix(memory): fix memory leaks in Jest JSDOM environment Jul 3, 2018
@FreeleX FreeleX changed the title fix(memory): fix memory leaks in Jest JSDOM environment fix(memory): reduce memory leaks in Jest JSDOM environment Jul 3, 2018
@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 3, 2018

@JiaLiPassion Replaced commit with new solution. I'll try to investigate why some tests failed later. In case you have some suggestions how to fix them please feel free to comment.

Copy link
Collaborator

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add the check logic here

if (!desc || !desc.configurable) {
    return;
}
if (!obj[zoneSymbol(`on` + prop + 'patched')]) {
  return;
}

So if already patched, we just return.

@FreeleX FreeleX force-pushed the memory-leaks-in-jsdom-fix branch 2 times, most recently from 68f0bae to 207b685 Compare July 3, 2018 18:42
@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 3, 2018

@JiaLiPassion done, but this didn't fix tests unfortunately

@FreeleX FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 207b685 to 465f1c2 Compare July 3, 2018 21:10
@@ -158,6 +158,11 @@ export function patchProperty(obj: any, prop: string, prototype?: any) {
return;
}

const onPropPatchedSymbol = zoneSymbol('on' + prop + 'patched');
if (obj[onPropPatchedSymbol]) {
// return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The comment was to see if tests are OK in case of no restriction

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, so now your test cases can work without memory leak?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, judging by heap usage log - memory leak disappeared both in test repo and in huge production project.
Despite the fact that Jest with --detectLeaks reports there are still leaks, again, judging by the heap usage log they are minor.
I have attached log with more accurate measurement for test repo as PR comment here, please take a look.

So I think this solution is viable, provided we fix tests' failures for PR in this (zone.js) repo.

@JiaLiPassion
Copy link
Collaborator

@FreeleX, I just added a comment, you should remove the comment of // return, I have tested with your repo, the memory will not increase.

@FreeleX FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 465f1c2 to bcc4719 Compare July 4, 2018 05:33
@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 4, 2018

I have changed the behavior in test repo to get more accurate measures by running GC after each test suite.
Please take a look at FreeleX/jest-angular-memory-leak-example@f500bfb for more details

With fix in this PR the results now are:

> jest --runInBand --logHeapUsage

 PASS  __tests__/test8.spec.js (50 MB heap size)
 PASS  __tests__/test1.spec.js (47 MB heap size)
 PASS  __tests__/test7.spec.js (50 MB heap size)
 PASS  __tests__/test19.spec.js (49 MB heap size)
 PASS  __tests__/test10.spec.js (49 MB heap size)
 PASS  __tests__/test15.spec.js (49 MB heap size)
 PASS  __tests__/test23.spec.js (49 MB heap size)
 PASS  __tests__/test6.spec.js (48 MB heap size)
 PASS  __tests__/test25.spec.js (49 MB heap size)
 PASS  __tests__/test22.spec.js (49 MB heap size)
 PASS  __tests__/test13.spec.js (49 MB heap size)
 PASS  __tests__/test24.spec.js (49 MB heap size)
 PASS  __tests__/test17.spec.js (49 MB heap size)
 PASS  __tests__/test2.spec.js (49 MB heap size)
 PASS  __tests__/test20.spec.js (49 MB heap size)
 PASS  __tests__/test14.spec.js (49 MB heap size)
 PASS  __tests__/test9.spec.js (49 MB heap size)
 PASS  __tests__/test21.spec.js (49 MB heap size)
 PASS  __tests__/test5.spec.js (49 MB heap size)
 PASS  __tests__/test16.spec.js (49 MB heap size)
 PASS  __tests__/test3.spec.js (49 MB heap size)
 PASS  __tests__/test12.spec.js (49 MB heap size)
 PASS  __tests__/test4.spec.js (49 MB heap size)
 PASS  __tests__/test18.spec.js (49 MB heap size)
 PASS  __tests__/test11.spec.js (49 MB heap size)

Test Suites: 25 passed, 25 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        2.996s
Ran all test suites.

Without fix:

> jest --runInBand --logHeapUsage

 PASS  __tests__/test8.spec.js (46 MB heap size)
 PASS  __tests__/test19.spec.js (47 MB heap size)
 PASS  __tests__/test15.spec.js (50 MB heap size)
 PASS  __tests__/test13.spec.js (53 MB heap size)
 PASS  __tests__/test7.spec.js (55 MB heap size)
 PASS  __tests__/test22.spec.js (59 MB heap size)
 PASS  __tests__/test25.spec.js (62 MB heap size)
 PASS  __tests__/test24.spec.js (65 MB heap size)
 PASS  __tests__/test10.spec.js (68 MB heap size)
 PASS  __tests__/test20.spec.js (72 MB heap size)
 PASS  __tests__/test17.spec.js (75 MB heap size)
 PASS  __tests__/test23.spec.js (78 MB heap size)
 PASS  __tests__/test12.spec.js (81 MB heap size)
 PASS  __tests__/test6.spec.js (85 MB heap size)
 PASS  __tests__/test2.spec.js (88 MB heap size)
 PASS  __tests__/test1.spec.js (91 MB heap size)
 PASS  __tests__/test4.spec.js (94 MB heap size)
 PASS  __tests__/test11.spec.js (98 MB heap size)
 PASS  __tests__/test5.spec.js (101 MB heap size)
 PASS  __tests__/test3.spec.js (104 MB heap size)
 PASS  __tests__/test18.spec.js (107 MB heap size)
 PASS  __tests__/test14.spec.js (111 MB heap size)
 PASS  __tests__/test21.spec.js (114 MB heap size)
 PASS  __tests__/test16.spec.js (117 MB heap size)
 PASS  __tests__/test9.spec.js (121 MB heap size)

Test Suites: 25 passed, 25 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        3.702s
Ran all test suites.

Also Jest with --detectLeaks still reports there are memory leaks,
but, judging by the log, this fix removed significant part / almost all of memory leaks

@JiaLiPassion
Copy link
Collaborator

@FreeleX , yeah, that's great, and without zone.js, jest --detectLeaks will not report any memory leak, is that correct?

@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 4, 2018

@JiaLiPassion yes, exactly, reports NO leaks when zone.js is NOT included/require-d/loaded.

@JiaLiPassion
Copy link
Collaborator

@FreeleX , I use chromedev tools to capture the memory and did some comparsion, it seems every test will create a new process/Window/Zone object and will not release them. I think maybe we need to implement mockClear callback for zone in jest, I will do more research later.

But I think current PR is good to go, further modification should be outside of zone.js, maybe in some library called zone-jest-patch because load zone.js multiple times is not a standard use case for zone.js.

@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 5, 2018

@JiaLiPassion Yes, I also thought about lack of unload mechanism in zone.js. It would be good to have some explicit load/unload in order to be able to control more precisely this process, like in Jest situation, but of course this is just something to conside for the future.

@JiaLiPassion
Copy link
Collaborator

@FreeleX, unload is under discussion here #1073, I will try to find out is there any walk around to fix the remain memory leak of jest.

@FreeleX FreeleX force-pushed the memory-leaks-in-jsdom-fix branch 8 times, most recently from 4cc53dc to 8f9c32e Compare July 11, 2018 18:44
@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 11, 2018

@JiaLiPassion I have fixed the code to pass tests, but one step stil fails. Could you please take a look at CI log and share some thoughts why it failed? Thanks

  This caused memory leaks in Jest JSDOM environment
@FreeleX
Copy link
Contributor Author

FreeleX commented Jul 13, 2018

@JiaLiPassion Looks like now tests are OK. Is it ready to be merged?)

@JiaLiPassion
Copy link
Collaborator

@FreeleX, this PR LGTM, and we need to let @mhevery to review and merge, thank you for the PR.

@mhevery mhevery merged commit 875086f into angular:master Jul 17, 2018
@FreeleX FreeleX deleted the memory-leaks-in-jsdom-fix branch July 18, 2018 06:14
@etavener
Copy link

Hey, thanks guys for fixing this. When will this be released? I'm really keen to get this memory leak fix.

@peterbakonyi05
Copy link

peterbakonyi05 commented Jul 19, 2018

Just as a note: we were also experiencing this heap out of memory issue only when --coverage was enabled. I tested locally, when I build current master branch of zone.js and replace it in node_modules folder, specs are running faster and there is no heap out of memory issue

@JiaLiPassion
Copy link
Collaborator

@peterbakonyi05, thank you for confirmation.
@etavener, I am not sure about the release date, you can use current main branch to build and test.

@etavener
Copy link

@JiaLiPassion I've checked it locally and it works like a gem. The sooner this is released and the sooner i take the NODE_OPTIONS=--max_old_space_size=4096 out of my project, the better. Thank you.

@GoGoris
Copy link

GoGoris commented Oct 4, 2018

Sorry for the late reply but I tried the latest version on master and I am still having the memory leak.
This is when I run it in the example repository:

  "devDependencies": {
    "jest": "^23.1.0",
    "weak": "^1.0.1",
    "zone.js": "github:angular/zone.js#7201d44451f6c67eac2b86eca3cbfafde14035d6"
  }
$ jest --logHeapUsage --runInBand
 PASS  __tests__/test2.spec.js (42 MB heap size)
 PASS  __tests__/test22.spec.js (48 MB heap size)
 PASS  __tests__/test19.spec.js (51 MB heap size)
 PASS  __tests__/test21.spec.js (52 MB heap size)
 PASS  __tests__/test17.spec.js (57 MB heap size)
 PASS  __tests__/test18.spec.js (59 MB heap size)
 PASS  __tests__/test1.spec.js (63 MB heap size)
 PASS  __tests__/test11.spec.js (66 MB heap size)
 PASS  __tests__/test8.spec.js (70 MB heap size)
 PASS  __tests__/test14.spec.js (73 MB heap size)
 PASS  __tests__/test9.spec.js (75 MB heap size)
 PASS  __tests__/test13.spec.js (78 MB heap size)
 PASS  __tests__/test20.spec.js (81 MB heap size)
 PASS  __tests__/test3.spec.js (84 MB heap size)
 PASS  __tests__/test10.spec.js (88 MB heap size)
 PASS  __tests__/test7.spec.js (91 MB heap size)
 PASS  __tests__/test15.spec.js (94 MB heap size)
 PASS  __tests__/test12.spec.js (100 MB heap size)
 PASS  __tests__/test16.spec.js (102 MB heap size)
 PASS  __tests__/test23.spec.js (105 MB heap size)
 PASS  __tests__/test6.spec.js (110 MB heap size)
 PASS  __tests__/test25.spec.js (112 MB heap size)
 PASS  __tests__/test5.spec.js (113 MB heap size)
 PASS  __tests__/test24.spec.js (118 MB heap size)
 PASS  __tests__/test4.spec.js (121 MB heap size)

Test Suites: 25 passed, 25 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        8.695s, estimated 31s
Ran all test suites.

Is there a regression in one of the latest commits?

@JiaLiPassion
Copy link
Collaborator

@GoGoris, the current master will still the 0.8.26 status, because /dist folder is not updated, you can try this branch.

"zone.js": "git://github.com/JiaLiPassion/zone.js#20181005-dist"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants