fix: use graceful-fs all over instead of monkey patching fs #9443

SimenB · 2020-01-22T13:49:32Z

Summary

From graceful-fs's readme:

This should only ever be done at the top-level application layer, in order to delay on EMFILE errors from any fs-using dependencies. You should not do this in a library, because it can cause unexpected delays in other parts of the program.

This might cause errors if some dependency we use do a lot of fs operations, but if so we should send them a PR using graceful-fs themselves. And users hitting it can choose to manually gracefulify themselves if they want to.

Ref #8331.

Test plan

Green CI.

SimenB · 2020-01-22T14:09:59Z

CI OOMs - that's a bad sign...

SimenB · 2020-01-22T14:58:44Z

Seems to leak when running in band, running in workers and heap size seems stable. I don't have time to dig into this now. Seems to happen on master as well, so not sure why it explodes on CI now and didn't before...

kirillgroshkov · 2020-02-17T16:36:34Z

Does it make sense to maybe consider merging this as a fix: #8331
?

SimenB · 2020-02-24T11:47:12Z

Seems to fix the memory issues on CI, so graceful-fs definitely leaks. Should put together a reproduction for them

SimenB · 2020-03-29T01:04:58Z

I've opened a PR at graceful-fs that seemingly fixes this: isaacs/node-graceful-fs#184

codecov-io · 2020-03-29T09:54:52Z

Codecov Report

Merging #9443 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9443   +/-   ##
=======================================
  Coverage   64.92%   64.92%           
=======================================
  Files         288      288           
  Lines       12199    12199           
  Branches     3024     3024           
=======================================
  Hits         7920     7920           
+ Misses       3639     3638    -1     
- Partials      640      641    +1

Impacted Files	Coverage Δ
packages/babel-jest/src/index.ts	`50.00% <ø> (ø)`
packages/jest-cli/src/init/index.ts	`82.60% <ø> (ø)`
packages/jest-config/src/index.ts	`11.59% <ø> (ø)`
packages/jest-config/src/normalize.ts	`76.66% <ø> (ø)`
...ges/jest-config/src/readConfigFileAndSetRootDir.ts	`0.00% <ø> (ø)`
packages/jest-config/src/resolveConfigPath.ts	`92.00% <ø> (ø)`
packages/jest-haste-map/src/crawlers/node.ts	`86.66% <ø> (ø)`
packages/jest-haste-map/src/index.ts	`81.26% <ø> (ø)`
packages/jest-haste-map/src/lib/FSEventsWatcher.ts	`12.72% <ø> (ø)`
packages/jest-haste-map/src/lib/WatchmanWatcher.js	`0.00% <ø> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a4b073...64ac79a. Read the comment docs.

SimenB · 2020-03-29T10:12:39Z

packages/jest-runtime/src/cli/index.ts

@@ -24,10 +24,6 @@ export async function run(
  cliArgv?: Config.Argv,
  cliInfo?: Array<string>,
 ): Promise<void> {
-  const realFs = require('fs');
-  const fs = require('graceful-fs');
-  fs.gracefulify(realFs);


this is the fix, buried in fs -> graceful-fs import changes

ron23 · 2020-03-31T03:53:56Z

Data point: this solution makes the leak worse in our repo.
Without the fix, the heap size at the end of small subset of tests was ~500MB.
With the fix ~750MB.

SimenB · 2020-03-31T07:02:06Z

@ron23 did you include the patch for graceful-fs? It's the last commit.

The patch is the fix for the memory leaks, the changes in this PR without the patch is expected to make things worse since we load a new (leaking) graceful-fs each time (similar to what happens when people load graceful-fs in their tests).

ron23 · 2020-03-31T09:09:34Z

@ron23 did you include the patch for graceful-fs? It's the last commit.

Oops I have not, didn't realize I need to.
Anyways, I just tried that (via yarn link on the main jest folder and on jest-runtime and jest-cli) and I got the exact same results.
Am I missing something?
Just to be sure it's running with the right code, I added logging on top of your graceful-fs.js changes which I do see while running the tests so it should be linking correctly.
I did notice the initial test takes way longer to boot (almost a minute) and that the heap size is about 50MB bigger.

EDIT: my leak might be coming from different place as well, so don't take that as if your PR ain't working. If only I knew where the leak is coming from...

SimenB · 2020-03-31T09:57:36Z

For testing I recommend not to link anything (there's so many packages and inter-dependencies it's easy to miss some). This is easier:

checkout graceful-fs-directly branch
run yarn to ensure patch is applied and all code is built
uninstall jest from you project
install patch-package in your project, and copy over the patches directory to get the fix
ensure you only have a single version of graceful-fs in your tree
from your project run yarn patch-package to apply the patch, then run ../folder-with-cloned-jest/jest.

This reverts commit d87166f.

This reverts commit 8855c28.

SimenB · 2020-04-27T20:07:19Z

Upstream PR has been merged, just need to wait for a release and we can merge this

This reverts commit ad7e8b3.

This reverts commit d19bdb5.

Couple of things here: **require.cache** In version `v25.5.0`, jest introduced an [implementation](jestjs/jest#9841) of their own to the `require.cache` object. It seems that it doesn't handle caching `json` modules properly, which causes our code to fail because `mod.paths` is `undefined` when we are querying for `json` files that were required (for example `package.json` or `cloud-assembly.schema.json`). https://github.com/aws/aws-cdk/blob/07fe642e38118e24837b492fa182737fc41bb429/packages/%40aws-cdk/core/lib/private/runtime-info.ts#L66 ```console TypeError: Cannot read property 'map' of undefined ``` The "fix" was to add a null check to prevent failure when looking up modules who don't have the `paths` property defined. Note that this was only observed in test environments using `jest > v25.5.0`, not during actual runtime of `cdk synth`. This is because `jest` manipulates the built-in `require` module of `nodejs`. **graceful-fs** In version `v25.5.0`, jest [added](jestjs/jest#9443) a dependency on the `graceful-fs` package. The version they depend on (`4.2.4`) differs from the version that we bring (`4.2.3`). This caused the `graceful-fs` dependency that comes from `jest` no to be deduped by node at installation. In turn, this caused multiple copies of the library to be installed on disk. The `graceful-fs` package monkey patches the `process.chdir` and `process.cwd` functions with a cached version for better performance. For reasons not completely clear to us, the existence of multiple copies of the module, caused `process.cwd()` to return incorrect cached results, essentially always returning the directory that the process was started from, without consideration to calls to `process.chdir`. This broke `decdk` (and possibly many more) tests which rely on `process.chdir`. Fixes #7657

github-actions · 2021-05-11T08:06:38Z

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the cla signed label Jan 22, 2020

SimenB force-pushed the graceful-fs-directly branch from f2a03ac to 0d1b938 Compare January 22, 2020 13:49

SimenB mentioned this pull request Jan 22, 2020

adds freezeCoreModules configuration option to mitigate memory leaks #8331

Closed

SimenB requested review from jeysal and thymikee January 22, 2020 13:54

SimenB force-pushed the graceful-fs-directly branch 2 times, most recently from d614c6f to f36fad7 Compare January 22, 2020 14:44

SimenB force-pushed the graceful-fs-directly branch 2 times, most recently from d44b0d4 to 84bf591 Compare February 24, 2020 09:59

ron23 mentioned this pull request Mar 24, 2020

Memory leak related to require #9697

Closed

SimenB force-pushed the graceful-fs-directly branch from 84bf591 to 8dd5817 Compare March 29, 2020 09:37

SimenB commented Mar 29, 2020

View reviewed changes

SimenB force-pushed the graceful-fs-directly branch from 64ac79a to d8cd798 Compare April 13, 2020 12:52

SimenB force-pushed the graceful-fs-directly branch from 8293c0c to 37413b1 Compare April 27, 2020 08:36

SimenB added 6 commits April 27, 2020 11:04

fix: use graceful-fs all over instead of monkey patching fs

6dd7557

log heap usage

d19bdb5

force older graceful

9170198

Revert "force older graceful"

2d163ea

This reverts commit d87166f.

what about giving outer graceful?

ade89a8

Revert "what about giving outer graceful?"

facbfed

This reverts commit 8855c28.

SimenB added 2 commits April 27, 2020 11:04

add graceful-fs patch

ad7e8b3

new patch

88e41c3

SimenB force-pushed the graceful-fs-directly branch from 37413b1 to 88e41c3 Compare April 27, 2020 09:04

SimenB mentioned this pull request Apr 27, 2020

Performance loss between jest 25.1.0 and 25.2.7 #9776

Closed

SimenB added 4 commits April 28, 2020 19:44

Merge branch 'master' into graceful-fs-directly

fac94a0

Revert "add graceful-fs patch"

8044b1b

This reverts commit ad7e8b3.

bump graceful

766a329

move changelog entry to correct place

a2a7802

SimenB force-pushed the graceful-fs-directly branch from 19ae6d6 to a2a7802 Compare April 28, 2020 17:51

Revert "log heap usage"

2bf52e8

This reverts commit d19bdb5.

SimenB merged commit 7e37b0f into jestjs:master Apr 28, 2020

SimenB deleted the graceful-fs-directly branch April 28, 2020 18:23

iliapolo mentioned this pull request Apr 29, 2020

fix: multiple breakages due to jest version upgrade aws/aws-cdk#7667

Merged

JamieMagee mentioned this pull request Apr 29, 2020

build(deps): update jest monorepo to v25.5.1 renovatebot/renovate#6092

Merged

1 task

SimenB mentioned this pull request May 8, 2020

Memory allocation failure when running lots of tests with --runInBand in limited env (Travis) #2179

Closed

dependabot bot mentioned this pull request Mar 17, 2021

Bump jest from 25.3.0 to 25.5.4 newerton/gostack-template-fundamentos-node#10

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use graceful-fs all over instead of monkey patching fs #9443

fix: use graceful-fs all over instead of monkey patching fs #9443

SimenB commented Jan 22, 2020

SimenB commented Jan 22, 2020

SimenB commented Jan 22, 2020 •

edited

Loading

kirillgroshkov commented Feb 17, 2020

SimenB commented Feb 24, 2020 •

edited

Loading

SimenB commented Mar 29, 2020

codecov-io commented Mar 29, 2020 •

edited

Loading

SimenB Mar 29, 2020

ron23 commented Mar 31, 2020

SimenB commented Mar 31, 2020 •

edited

Loading

ron23 commented Mar 31, 2020 •

edited

Loading

SimenB commented Mar 31, 2020

SimenB commented Apr 27, 2020

github-actions bot commented May 11, 2021

fix: use graceful-fs all over instead of monkey patching fs #9443

fix: use graceful-fs all over instead of monkey patching fs #9443

Conversation

SimenB commented Jan 22, 2020

Summary

Test plan

SimenB commented Jan 22, 2020

SimenB commented Jan 22, 2020 • edited Loading

kirillgroshkov commented Feb 17, 2020

SimenB commented Feb 24, 2020 • edited Loading

SimenB commented Mar 29, 2020

codecov-io commented Mar 29, 2020 • edited Loading

Codecov Report

SimenB Mar 29, 2020

Choose a reason for hiding this comment

ron23 commented Mar 31, 2020

SimenB commented Mar 31, 2020 • edited Loading

ron23 commented Mar 31, 2020 • edited Loading

SimenB commented Mar 31, 2020

SimenB commented Apr 27, 2020

github-actions bot commented May 11, 2021

SimenB commented Jan 22, 2020 •

edited

Loading

SimenB commented Feb 24, 2020 •

edited

Loading

codecov-io commented Mar 29, 2020 •

edited

Loading

SimenB commented Mar 31, 2020 •

edited

Loading

ron23 commented Mar 31, 2020 •

edited

Loading