-
Notifications
You must be signed in to change notification settings - Fork 407
fix(memory): reduce memory leaks in Jest JSDOM environment #1106
Conversation
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. What to do if you already signed the CLAIndividual signers
Corporate signers
|
1 similar comment
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. What to do if you already signed the CLAIndividual signers
Corporate signers
|
c256963
to
043f18d
Compare
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
043f18d
to
07679eb
Compare
@FreeleX , I am not quite understand why there is a leak, and based on the comment angular/angular#24048 (comment), you said the |
@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). 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. Tested on https://github.com/FreeleX/jest-angular-memory-leak-example repo: Without patch:
With patch:
The patch is:
I've found out that I hope this will help with further investigation of the issue in some way :) Open to any suggestions. Thank you |
@FreeleX , thank you very much for the detailed researh, I will check it. |
@FreeleX , I run your sample, and it seems And to be consistant, please keep the
to be
Thank you for the research, it really helped. |
07679eb
to
ef7116f
Compare
@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. |
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.
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.
68f0bae
to
207b685
Compare
@JiaLiPassion done, but this didn't fix tests unfortunately |
207b685
to
465f1c2
Compare
lib/common/utils.ts
Outdated
@@ -158,6 +158,11 @@ export function patchProperty(obj: any, prop: string, prototype?: any) { | |||
return; | |||
} | |||
|
|||
const onPropPatchedSymbol = zoneSymbol('on' + prop + 'patched'); | |||
if (obj[onPropPatchedSymbol]) { | |||
// return; |
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.
please remove the comment.
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.
Done. The comment was to see if tests are OK in case of no restriction
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.
Got it, so now your test cases can work without memory leak?
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.
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.
@FreeleX, I just added a comment, you should remove the comment of |
465f1c2
to
bcc4719
Compare
I have changed the behavior in test repo to get more accurate measures by running GC after each test suite. With fix in this PR the results now are:
Without fix:
Also Jest with --detectLeaks still reports there are memory leaks, |
@FreeleX , yeah, that's great, and without |
@JiaLiPassion yes, exactly, reports NO leaks when zone.js is NOT included/require-d/loaded. |
@FreeleX , I use chromedev tools to capture the memory and did some comparsion, it seems every test will create a new But I think current PR is good to go, further modification should be outside of |
@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. |
4cc53dc
to
8f9c32e
Compare
@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
8f9c32e
to
cc95390
Compare
@JiaLiPassion Looks like now tests are OK. Is it ready to be merged?) |
Hey, thanks guys for fixing this. When will this be released? I'm really keen to get this memory leak fix. |
Just as a note: we were also experiencing this heap out of memory issue only when |
@peterbakonyi05, thank you for confirmation. |
@JiaLiPassion I've checked it locally and it works like a gem. The sooner this is released and the sooner i take the |
Sorry for the late reply but I tried the latest version on master and I am still having the memory leak.
Is there a regression in one of the latest commits? |
@GoGoris, the
|
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