fix(memory): reduce memory leaks in Jest JSDOM environment by FreeleX · Pull Request #1106 · angular/zone.js

FreeleX · 2018-06-26T18:40:14Z

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 · 2018-06-26T18:40:19Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-06-26T18:40:20Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-06-26T18:47:59Z

CLAs look good, thanks!

googlebot · 2018-06-26T18:47:59Z

CLAs look good, thanks!

JiaLiPassion · 2018-06-27T01:33:19Z

@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 · 2018-07-02T18:39:11Z

@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 · 2018-07-02T19:31:26Z

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

JiaLiPassion · 2018-07-02T20:42:57Z

@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 · 2018-07-03T08:42:15Z

@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.

JiaLiPassion

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 · 2018-07-03T20:26:26Z

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

JiaLiPassion · 2018-07-04T00:57:35Z


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


please remove the comment.

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

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

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 · 2018-07-04T00:58:21Z

@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 · 2018-07-04T08:05:45Z

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 · 2018-07-04T15:33:51Z

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

FreeleX · 2018-07-04T15:43:35Z

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

JiaLiPassion · 2018-07-05T02:50:36Z

@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 · 2018-07-05T17:51:16Z

@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 · 2018-07-06T01:43:46Z

@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 · 2018-07-11T19:45:24Z

@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 · 2018-07-13T15:54:04Z

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

JiaLiPassion · 2018-07-13T16:08:12Z

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

etavener · 2018-07-18T10:40:15Z

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

peterbakonyi05 · 2018-07-19T13:33:07Z

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 · 2018-07-19T15:16:02Z

@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 · 2018-07-19T15:51:37Z

@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 · 2018-10-04T14:45:11Z

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 · 2018-10-04T16:32:37Z

@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"

googlebot added the cla: no label Jun 26, 2018

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from c256963 to 043f18dCompare June 26, 2018 18:47

googlebot added cla: yes and removed cla: no labels Jun 26, 2018

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 patchingJun 26, 2018

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 043f18d to 07679ebCompare June 26, 2018 18:55

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 07679eb to ef7116fCompare July 3, 2018 07:44

FreeleX changed the title ~~fix(memory leaks in jsdom): wrap objects for safe prototypes patching~~fix(memory): fix memory leaks in Jest JSDOM environmentJul 3, 2018

FreeleX changed the title ~~fix(memory): fix memory leaks in Jest JSDOM environment~~fix(memory): reduce memory leaks in Jest JSDOM environmentJul 3, 2018

JiaLiPassion suggested changes Jul 3, 2018
View reviewed changes

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch 2 times, most recently from 68f0bae to 207b685Compare July 3, 2018 18:42

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 207b685 to 465f1c2Compare July 3, 2018 21:10

JiaLiPassion suggested changes Jul 4, 2018
View reviewed changes

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 465f1c2 to bcc4719Compare July 4, 2018 05:33

JiaLiPassion approved these changes Jul 5, 2018
View reviewed changes

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch 8 times, most recently from 4cc53dc to 8f9c32eCompare July 11, 2018 18:44

fix(memory): Add protection against excessive on prop patching
cc95390
This caused memory leaks in Jest JSDOM environment

FreeleX force-pushed the memory-leaks-in-jsdom-fix branch from 8f9c32e to cc95390Compare July 13, 2018 14:02

mhevery approved these changes Jul 17, 2018
View reviewed changes

mhevery merged commit 875086f into angular:masterJul 17, 2018

FreeleX deleted the memory-leaks-in-jsdom-fix branch July 18, 2018 06:14

FreeleX mentioned this pull request Aug 7, 2018
Angular TestBed memory leak angular/angular#24048
Closed

ricveal mentioned this pull request Oct 23, 2018
Memory leak when running tests thymikee/jest-preset-angular#186
Closed

peterbakonyi05 mentioned this pull request Oct 29, 2018
Question:When Can we expect new release of zone.js? #1149
Closed

Uh oh!

Conversation

FreeleX commented Jun 26, 2018

Uh oh!

googlebot commented Jun 26, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Jun 26, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Jun 26, 2018

Uh oh!

googlebot commented Jun 26, 2018

Uh oh!

JiaLiPassion commented Jun 27, 2018

Uh oh!

FreeleX commented Jul 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Jul 2, 2018

Uh oh!

JiaLiPassion commented Jul 2, 2018

Uh oh!

FreeleX commented Jul 3, 2018

Uh oh!

JiaLiPassion left a comment

Choose a reason for hiding this comment

Uh oh!

FreeleX commented Jul 3, 2018

Uh oh!

JiaLiPassionJul 4, 2018

Choose a reason for hiding this comment

Uh oh!

FreeleXJul 4, 2018

Choose a reason for hiding this comment

Uh oh!

JiaLiPassionJul 4, 2018

Choose a reason for hiding this comment

Uh oh!

FreeleXJul 4, 2018

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Jul 4, 2018

Uh oh!

FreeleX commented Jul 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Jul 4, 2018

Uh oh!

FreeleX commented Jul 4, 2018

Uh oh!

JiaLiPassion commented Jul 5, 2018

Uh oh!

FreeleX commented Jul 5, 2018

Uh oh!

JiaLiPassion commented Jul 6, 2018

Uh oh!

FreeleX commented Jul 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FreeleX commented Jul 13, 2018

Uh oh!

JiaLiPassion commented Jul 13, 2018

Uh oh!

etavener commented Jul 18, 2018

Uh oh!

peterbakonyi05 commented Jul 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Jul 19, 2018

Uh oh!

etavener commented Jul 19, 2018

Uh oh!

GoGoris commented Oct 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FreeleX commented Jul 2, 2018•
edited
Loading

FreeleX commented Jul 4, 2018•
edited
Loading

FreeleX commented Jul 11, 2018•
edited
Loading

peterbakonyi05 commented Jul 19, 2018•
edited
Loading

GoGoris commented Oct 4, 2018•
edited
Loading