adds freezeCoreModules configuration option to mitigate memory leaks #8331
Conversation
|
|
||
| Prevents overriding node's core module methods so to prevent memory leaks. | ||
|
|
||
| Attempt to override such a property will fail silently. |
There was a problem hiding this comment.
our > 10K test suite runs successfully with these changes, so i guess that in most cases, there is no actual need/use of these overrides.
|
|
||
| ### `freezeCoreModulesWhitelist` [Array<string>] | ||
|
|
||
| Default: `['crypto']` |
There was a problem hiding this comment.
Proxies have issues with bound native methods, hence crypto.randomBytes will fail. Tried to hack it from multiple directions with no success.
| @@ -374,6 +378,8 @@ export type ProjectConfig = { | |||
| extraGlobals: Array<keyof NodeJS.Global>; | |||
| filter: Path | null | undefined; | |||
| forceCoverageMatch: Array<Glob>; | |||
| freezeCoreModules: boolean; | |||
There was a problem hiding this comment.
not quite sure if these should be on ProjectConfig or GlobalConfig. probably GlobalConfig 😵 . kindly assist.
lev-kazakov
force-pushed
the
add-freeze-core-modules-configuration-option
branch
2 times, most recently
from
d89a5bb to
7bee921
Compare
|
@scotthovestadt thoughts on this one? |
|
@SimenB , @scotthovestadt |
|
That's not really an option - if you want that Jest is not for you :) Almost all of the features in jest depends on the isolation (mocks, transformations, fake timers etc). |
Codecov Report
@@ Coverage Diff @@
## master #8331 +/- ##
==========================================
+ Coverage 64.73% 64.96% +0.22%
==========================================
Files 277 283 +6
Lines 11709 12104 +395
Branches 2876 2990 +114
==========================================
+ Hits 7580 7863 +283
- Misses 3512 3604 +92
- Partials 617 637 +20
Continue to review full report at Codecov.
|
|
I've made a patch-package of this pull request if more people wants to test this tests against 24.7.1 https://gist.github.com/sibelius/f98e62dd9346ddc97f07fe3814dc1b6e |
|
More me the main aggressor is Error: Trying to override method 'request' of a frozen core module 'https'
at Object.<anonymous> (/app/node_modules/@slack/client/node_modules/agent-base/patch-core.js:45:15) |
|
using this pull requests keeps my tests below 200MB |
|
@SimenB , @scotthovestadt |
|
any updates on this PR? It's a pain to debug memory leaks due to hotpatching in some deep dependency and it only affects us when testing. |
|
Is this going to be merged? We'd really benefit a lot from this PR over here. |
|
@SimenB , @scotthovestadt any thoughts on this one? |
packages/jest-runtime/src/index.ts
Outdated
| value: any, | ||
| receiver: any, | ||
| ) => { | ||
| if (typeof value !== 'function') { |
There was a problem hiding this comment.
Here I had to also avoid caching in a case when value._isMockFunction was true, otherwise jest.spyOn would refuse to work (i.e. jest.spyOn(fs, "readFileSync"))
|
@lev-kazakov can you rebase? |
lev-kazakov
force-pushed
the
add-freeze-core-modules-configuration-option
branch
from
105fc9b to
bde98f6
Compare
|
@sibelius done. |
|
@SimenB , @scotthovestadt is there a chance we get this thing reviewed/merged/denied/whatever? :) |
|
Ping @SimenB , @scotthovestadt any time to look at this PR? We have reached the point at which we are trying to avoid upgrading packages in our main app because of the potential leaks that might happen during testing 😬 |
|
Hi @lev-kazakov @dir01 , I am from a company that is heavily using Jest for unit testing. In our test, we have a requirement to use 'setupFiles' config to load some large other 3rd party dependencies into the global variable. Hence, our test suites are suffering great memory leak (mostly due to grateful-fs). Thanks, |
|
Hi @lev-kazakov, Our team was able to temporarily fix our memory leak issues by using your fix in the CI, we just have one question:
We did find some problems with this fix. In our test, we are using Nock: https://github.com/nock/nock We are looking forward to hearing some thoughts from you, and are very interested in whether this change is planning to be merged. Regards, |
|
Hi @yang6lang1, |
|
Thanks for the reply! Unfortunately, I haven’t been able to track down which modules are leaking. There are no console warning or errors when using the patch. Using Edit: fwiw, I have |
|
try to profile both cpu and memory like this
|
|
I think this option can dramatically improve experience in finding modules, that are leaking. |
then-fs seems to be causing a memory leak in jest in other tools. Found by jestjs/jest#8331
|
This has helped me find I feel that this useful contribution allows me also to add a "plus one", which follows. I spent probably days trying to find this with a heap debugger. These memory leaks are a killer for us; with We work around this by splitting the test run manually, completely negating most of the benefits of A "quick fix" for this problem for us is to use the above Jest patch to find places where we're importing something, and change it to a lazy becomes ...totally ruining our TS and linting rules and, in some places, the code structure. |
|
I think this could be useful to find leaks |
then-fs seems to be causing a memory leak in jest in other tools. Found by jestjs/jest#8331
then-fs monkey patches the stdlib, which causes problems for jest, c.f. jestjs/jest#8331 promise-fs is a drop-in replacement here, for us.
then-fs seems to be causing a memory leak in jest in other tools. Found by jestjs/jest#8331
|
jest@26.1.0 does not have more memory leaks for our codebase this patch was breaking |
|
latest version with node 14 still leaking I think it is graceful-fs again |
|
can we rebase this once again? |
hi, sorry, no capacity. |
@lev-kazakov Just be clear, the jest25.5 or ts-jest 25.5 or jest-core25.5. I will fix my verison |
i think you meant to tag @SimenB. i'm not part of the |
I like using your branch as a temp solution, till that merges into master. But still not very understand why
cause memory leak |
|
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
Dear github bot, please don't close this useful PR, until we finally have a better solution for the huge memory leaks in jest or ts-jest Sincerely, |
|
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
resolves #6399, resolves #6814
Summary
Some commonly used libraries like agent-base and graceful-fs override node core modules' (a.k.a built-in modules) methods.
This causes a memory leak in jest since the long lived node core modules' state is now bound to the short lived test execution context.
Each time a new test runs, jest creates a new context, the context is then bound to the long lived core module state, thus increasing the leak.
Test plan
Tests were added here:
https://github.com/lev-kazakov/jest/blob/25fb8ba198878bef6f8b25baac0b79ce57f33fda/packages/jest-runtime/src/__tests__/runtime_require_module.test.js#L186-L211
For an e2e test, you can run the following test suite with this branch linked:
https://github.com/lev-kazakov/jest-leak-fixer/tree/master/test