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

Memory leak issue with jest #1817

Closed
juhosuominen opened this issue Nov 15, 2019 · 21 comments · Fixed by #1962
Closed

Memory leak issue with jest #1817

juhosuominen opened this issue Nov 15, 2019 · 21 comments · Fixed by #1962

Comments

@juhosuominen
Copy link

What is the expected behavior?
Node heap size shouldn't grow in every test.

What is the actual behavior?
Heap size increases after every test file.

With nock:

PASS  test/function-86.js (22 MB heap size)
PASS  test/function-75.js (22 MB heap size)
PASS  test/function-1.js (22 MB heap size)
PASS  test/function-43.js (26 MB heap size)
PASS  test/function-24.js (26 MB heap size)
...
PASS  test/function-48.js (133 MB heap size)
PASS  test/function-82.js (137 MB heap size)
PASS  test/function-87.js (137 MB heap size)
PASS  test/function-20.js (137 MB heap size)
PASS  test/function-79.js (140 MB heap size)

Test Suites: 100 passed, 100 total
Tests:       100 passed, 100 total
Snapshots:   0 total
Time:        25.506s, estimated 26s

Without nock:

PASS  test/function-86.js (19 MB heap size)
PASS  test/function-75.js (19 MB heap size)
PASS  test/function-1.js (19 MB heap size)
PASS  test/function-65.js (19 MB heap size)
PASS  test/function-96.js (19 MB heap size)
...
PASS  test/function-93.js (20 MB heap size)
PASS  test/function-61.js (20 MB heap size)
PASS  test/function-49.js (20 MB heap size)
PASS  test/function-87.js (20 MB heap size)
PASS  test/function-79.js (20 MB heap size)

Test Suites: 100 passed, 100 total
Tests:       100 passed, 100 total
Snapshots:   0 total
Time:        27.079s

How to reproduce the issue

Even a single require/import of nock seems to cause the issue.

Minimal test case: https://github.com/juhosuominen/jest-nock-memory-leak

Versions

Software Version(s)
Nock 11.7.0
Node 11.15.0
@mastermatt
Copy link
Member

@juhosuominen thanks for this. I've started to play with your test repo a bit, but I'll need to spend more time with it this weekend.

One thing I've noticed is that I can't reproduce your statement "Even a single require/import of nock seems to cause the issue.".
While I do get the same results listed above using npm run test (large heap size) and npm run test:no-nock (steady heap size), if I just add require('nock'), unconditionally, to one of the tests then run npm run test:no-nock I don't see the heap get large.

So on my machine simply importing Nock isn't the issue, it's the repeated use of Nock that does it.
@juhosuominen can you confirm you are experience different?

Doing a quick dive into a heap dump points to Babel/generator, but I need to more time exploring to see if that is a red herring.

@mastermatt
Copy link
Member

Ahh now I see what your statement means. A single require of Nock in each test suite causes the issue.
Yes, I can reproduce this. Having many Jest test suite whose contents are simply the following demonstrates the leak

require('nock');

it('should do nothing', async () => {
	// do nothing
});

So far I've been able to determine that this has to do with Jest not respecting the require.cache across suites (ref, example) and the fact that Nock mutates the global http module on import. A known shortcoming.

I'm not sure why there is a memory leak. I'm still leaning towards something related to Babel.

@paulmelnikow
Copy link
Member

Just a hunch, any chance invoking nock.restore() after each test makes a difference?

We do keep a reference to the original http & https functions inside nock.

Though I’m still not really clear on what Jest is doing, or how the combination of Jest and Nock creates leaks.

@mastermatt
Copy link
Member

yes, I have seen that adding nock.restore() at the end of each suite completely fixes the leak.

Right now I'm playing around with different ways of tacking on the original ClientRequest onto a global so Nock doesn't keep extending its own subclass. But I haven't figured out the quirks of Jest's and/or Babel's caching mechanisms yet.

I'm going to call it a night.

@paulmelnikow
Copy link
Member

We end up extending our own subclass over and over?

@mastermatt
Copy link
Member

We end up extending our own subclass over and over?

yes.

@mastermatt
Copy link
Member

It's hard to call this a bug, since Jest is the one breaking from how Node is suppose to work.

@mastermatt
Copy link
Member

What's confusing is that if I do the dance to store the original ClientRequest on the http lib then reuse that to inherit from, the leak is still present. It seems Babel Generator notices the class has changed and re-processes it, while keeping a ref to the old classes.

This issue also exists for the http[s].request and get methods.

@mastermatt
Copy link
Member

The discussion in Jest jestjs/jest#6814

And a nice write up on it. https://chanind.github.io/javascript/2019/10/12/jest-tests-memory-leak.html

@paulmelnikow
Copy link
Member

I wonder if we could detect Jest and then print a warning if we try to extend and discover that it’s already been overridden. We could also automatically reset but I don’t like that idea too much; at least not without understanding better what Jest is doing.

Another option is to avoid patching if it’s already patched by setting a flag in the global module. (Perhaps a flag works where saving original request doesn’t?) Though I wonder if that would work. Nock has its own global state, and I’m not quite clear whether request patched in a previous test would work with mocks defined in a new test.

@juhosuominen
Copy link
Author

@mastermatt Yes, it seems I was mistaken about the "Even a single require/import of nock seems to cause the issue" part. We were probably importing nock in some test boilerplate file.

I did manage to fix the memory leak by calling nock.restore() after each test suite as you suggested.

@stale
Copy link

stale bot commented Feb 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

@stale stale bot added the stale label Feb 16, 2020
@kirillgroshkov
Copy link

Any update on this?

@stale stale bot removed the stale label Feb 17, 2020
@paulmelnikow
Copy link
Member

I did manage to fix the memory leak by calling nock.restore() after each test suite as you suggested.

Isn't this the fix?

@kirillgroshkov
Copy link

I did manage to fix the memory leak by calling nock.restore() after each test suite as you suggested.

Isn't this the fix?

Yes, probably. I wasn't able to confirm it (Jest still reports leaks and actually leaks 100s of Mbs of memory per test file). But that's probably due to other dependencies I have. So, ignore my last comment please:)

@kirillgroshkov
Copy link

Just a question, will nock.cleanAll() also do the job, or only nock.restore()?

@mastermatt
Copy link
Member

cleanAll removes out all the registered interceptors, but does not remove the monkey patched functions and class from the builtins. So it won't be enough in this case.

@kirillgroshkov
Copy link

cleanAll removes out all the registered interceptors, but does not remove the monkey patched functions and class from the builtins. So it won't be enough in this case.

Thanks!

@mainfraame
Copy link

this is still an issue. I migrated to xhr mock and my issues went away. The authors of this library should take a look at what graceful-fs and agent-base did to resolve the issue. A great thread about it can be found here

@mastermatt
Copy link
Member

@mentierd yes, at this time we don't plan on "fixing" this as it's an issue with Jest. jestjs/jest#6814 as noted in the comment above. #1817 (comment).

We have updated our docs to describe how to avoid this when using Nock in Jest suites.
Memory issues with Jest

Unfortunately, the comment just above that one describes an attempt to get around it, in the same way graceful-fs and agent-base did. Those libs have slightly different use cases. graceful-fs was caching whole stdlib modules and agent-base was only monkey patching https.request. Nock overrides http.ClientRequest and the underlaying implementation of Babel, that Jest utilizes, maintains a reference to the child classes and reprocesses the class even when the override is skipped on subsequent activations of Nock.

If anyone comes across a way to get around this or force Babel to stop its caching in a way that doesn't make things worse, we'd love a PR.

@RedTn
Copy link

RedTn commented Jul 28, 2022

This still seems to be an issue, though I can confirm that when I use node 11.15.0 and jest ^26 that there is no leak. As soon as I upgrade to node 16 (16.10.0) this is an issue again
#2383

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 a pull request may close this issue.

6 participants