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

fix(ngMock): prevent memory leak due to data attached to `$rootElement` #14098

Closed

Conversation

Projects
None yet
4 participants
@gkalpak
Copy link
Member

gkalpak commented Feb 20, 2016

Starting with 88bb551, ngMock will attach the $injector to the $rootElement, but will never clean it up, resulting in a memory leak. Since a new $rootElement is created for every test, this leak causes Karma to crash on large test-suites.
The problem was not detected by our internal tests, because we do our own clean-up in
testabilityPatch.js.

This commit prevents the memory leak, by cleaning up all data attached to $rootElement after each test.

Fixes #14094

describe('make sure that we can create an injector outside of tests', function() {
//since some libraries create custom injectors outside of tests,
//we want to make sure that this is not breaking the internals of
//how we manage annotated function cleanup during tests. See #10967
angular.injector([function($injector) {}]);
});


describe('don\'t leak memory between tests', function() {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 21, 2016

Member

I prefer describe block descriptions to be nouns of things that are being tested...

But I see you are following the pattern above :-(

This comment has been minimized.

Copy link
@gkalpak

gkalpak Feb 21, 2016

Author Member

I don't like the descriptions either. I'll tweak them.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Feb 21, 2016

So the idea of the tests is that you have to run a test beforehand that sets up the spies so that you can check to see that the root element was in fact cleaned up?
I think that the wording of the describe and it blocks could be tweaked so that we remember what they are doing in 12 months time.

});
});

describe('don\'t leak memory between tests with mocked `$rootScope`', function() {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 21, 2016

Member

should this be "with mocked $rootElement"?

This comment has been minimized.

Copy link
@gkalpak

gkalpak Feb 21, 2016

Author Member

Yup :)

expect(cleanUpElems.length).toBe(1);
expect(cleanUpElems[0][0]).toBe(prevRootElement[0]);
}
});

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 21, 2016

Member

Do we need to do anything to tidy up the spies that we are holding on to?

This comment has been minimized.

Copy link
@gkalpak

gkalpak Feb 21, 2016

Author Member

Theoretically, no. Jasmine will restore the original objects after each test. We hold on to the spies, but the original object is restored. When the describe blocks are done, the references to those spies are useless (and should be garbage collected), but don't affect the environment in any way.

if (elem && elem.off) elem.off();
if (elem && elem.removeData) elem.removeData();
});
if (window.jQuery) window.jQuery.cleanData(cleanUpElems);

This comment has been minimized.

Copy link
@jbedard

jbedard Feb 21, 2016

Contributor

I think this can be jqLite.cleanData(cleanUpElements). Then you might not need the off or removeData calls above?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 21, 2016

Member

I was wondering about that too.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Feb 21, 2016

Author Member

I copied this over from testabilityPatch.js, but I think you are right.

From a quick look, I alos think there might be a leak here (and in testabilityPatch.js):
Passing an array of jqLite/jQuery elements won't properly clean up. cleanData expects an array of "raw" nodes.

I'll try this out.

This comment has been minimized.

Copy link
@jbedard

jbedard Feb 22, 2016

Contributor

The jqLite version of cleanData was added fairly recently...

This comment has been minimized.

Copy link
@gkalpak

gkalpak Feb 22, 2016

Author Member

Yeah, that would explain it :)

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch from 44684c9 to 393f68b Feb 21, 2016

fix(ngMock): prevent memory leak due to data attached to `$rootElement`
Starting with 88bb551, `ngMock` will attach the `$injector` to the `$rootElement`, but will never
clean it up, resulting in a memory leak. Since a new `$rootElement` is created for every test,
this leak causes Karma to crash on large test-suites.
The problem was not detected by our internal tests, because we do our own clean-up in
`testabilityPatch.js`.

This commit prevents the memory leak, by cleaning up all data attached to `$rootElement` after each
test.

Fixes #14094

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch from 393f68b to 739d4c6 Feb 21, 2016

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Feb 21, 2016

I has to rebase this on top of master (because the commit this is supposed to fix was reverted).

I incorporated the original commit (attaching $injector to $rootScope) in this PR's 1st commit. (Other than that, I haven't done any other changes in the 1st commit, so you don't have to re-review the other parts.)

I added a 2nd commit, that addressed your feedback (except for #14098 (comment) - I'll play with that next).
I tried to make the test descriptions as clear as possible and add comments explaining parts of the test logic that felt unclear, but I'm open to suggestions for better explanations 😃

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch 2 times, most recently from fd1891d to a33e753 Feb 21, 2016

fixup 1
- Group into a `describe` block
- Tweak test descriptions
- Fix typos

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch from a33e753 to b972a12 Feb 21, 2016

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Feb 21, 2016

I added a 3rd commit that uses jqLite.cleanData(...).

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch from 9e62160 to d8fca15 Feb 22, 2016

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Feb 22, 2016

And a 4th commit (fixup 3) that fixes a couple of corner cases.
(BTW, the commit messages contain more details about the contents of each fixup commit.)

fixup 2
- Unify the clean-up paths for tests with/without jQuery, using `angular.element.cleanData()`.
- Remove redundant calls to `.off().removeData()`.
- Fix bug in `testabilityPatch.js`, where `cleanData()` was called with an array of jqLite/jQuery
  elements (instead of "raw" nodes).

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch from 339468f to 352e2a9 Feb 22, 2016

fixup 3
- Reset `originalRootElement` for each test to avoid.
- Fix clean-up break, if `$rootElement` was never initialized.
- Fix clean-up break, if decorated `$rootElement` was falsy (e.g. `null`).
- (Rename `cleanUpElems` to `cleanUpNodes` - since it's "raw" nodes, not jq-wrapped elements.)

@gkalpak gkalpak force-pushed the gkalpak:fix-ngMock-prevent-memory-leak branch 2 times, most recently from 223fb52 to db92324 Feb 22, 2016

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Feb 22, 2016

LGTM - please squash and merge.

@gkalpak gkalpak closed this in 85ef70f Feb 22, 2016

gkalpak added a commit that referenced this pull request Feb 22, 2016

fix(ngMock): prevent memory leak due to data attached to `$rootElement`
Starting with 88bb551, `ngMock` will attach the `$injector` to the `$rootElement`, but will never
clean it up, resulting in a memory leak. Since a new `$rootElement` is created for every test,
this leak causes Karma to crash on large test-suites.
The problem was not detected by our internal tests, because we do our own clean-up in
`testabilityPatch.js`.

88bb551 was revert with 1b8590a.
This commit incorporates the changes from 88bb551 and prevents the memory leak, by cleaning up all
data attached to `$rootElement` after each test.

Fixes #14094

Closes #14098

gkalpak added a commit that referenced this pull request Feb 22, 2016

fix(ngMock): prevent memory leak due to data attached to `$rootElement`
Starting with 88bb551, `ngMock` will attach the `$injector` to the `$rootElement`, but will never
clean it up, resulting in a memory leak. Since a new `$rootElement` is created for every test,
this leak causes Karma to crash on large test-suites.
The problem was not detected by our internal tests, because we do our own clean-up in
`testabilityPatch.js`.

88bb551 was revert with 1b8590a.
This commit incorporates the changes from 88bb551 and prevents the memory leak, by cleaning up all
data attached to `$rootElement` after each test.

Fixes #14094

Closes #14098
@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Feb 22, 2016

Backported to v1.5.x (75373dd) and v1.4.x (571e323).
(For v.1.4.x I had to also backport jqLiteCleanData() from 96288d0.)

@gkalpak gkalpak deleted the gkalpak:fix-ngMock-prevent-memory-leak branch Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.