search

tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
911 stars 86 forks source link

fix: fix the memory leak introduced in nodejs v10 patch #303

Closed 3cp closed 4 years ago

3cp commented 4 years ago

closes #302

nonara commented 4 years ago

Looking into this now. First results were inconclusive. It looked like it was working at first, but one of the threads still climbs over 1gb, which doesn't seem right to me.

So, I'm doing some memory profiling now. Going to examine heap snapshots. Will let you know!

nonara commented 4 years ago

Looks good!

I pre-loaded it with one large file (149,382,687 bytes)

This is the heap comparison of before -> after mock.restore().

image

For good measure, I also mocked & restored repeatedly with the same data about 6 times in a row. Reported memory use in task manager stayed the same.

Thanks for the fast work! BTW, Have you had a chance to look at my PR?

3cp commented 4 years ago

@tschaub can we merge this?

tschaub commented 4 years ago

@3cp - yes please. Are you able to? Every time I get the notification, I’m not on a device that is allowed to merge with the failed test.

3cp commented 4 years ago

I cannot. Tried to skip win32 node 12 in CI, but GitHub is not happy about it, maybe I missed some config.

tschaub commented 4 years ago

Thanks for the fix, @3cp. Sorry it took so long to get in.

harryhorton commented 4 years ago

Can we get a release 🙏 ?