search

sindresorhus / import-fresh

Import a module while bypassing the cache
MIT License
284 stars 25 forks source link

Fix memory leak in require cache module (fixes #2) #3

Closed adammockor closed 7 years ago

adammockor commented 8 years ago

Hello,

this PR is an attempt to fix memory leak with require described in issue #2.

Please let me now if it is ok and merge-able.

Thank you.

adammockor commented 8 years ago

Thank you for your feedback.

More information about my problem can be found https://github.com/eslint/eslint/issues/7109.

I am using your test to confirm functionality and I also used eslint tests suite to confirm functionality in larger project.

Should I write dedicated test to test contents of require.cache module?

sindresorhus commented 8 years ago

Ideally, there needs to be a test that confirms the memory leak, so to prevent future regressions.

adammockor commented 8 years ago

Prevent future regressions is hard think to do, because this module relies on unofficial require API, so it can be changed anytime. There is no official way of doing this (https://github.com/nodejs/node/issues/8443#issuecomment-247384217).

There is a way of testing memory heap diffs via https://github.com/marcominetti/node-memwatch, but I rather don't do this, because these kind of modules are really not stable to use programatically in different environments (I am currently unable to install it on node 6.5.0 and I have issues with this at past).

So there is working your end-to-end test.

I can create test to check if require.cache supports parent child relationship and inspect cache for module availability.

I don't have any more ideas, what more can be done :)

sindresorhus commented 8 years ago

Prevent future regressions is hard think to do, because this module relies on unofficial require API, so it can be changed anytime.

No, the module system is locked. It won't ever change.

I don't have any more ideas, what more can be done :)

Add a file that can be manually run that visibly leaks memory, so I can manually confirm. I need proof that your change actually fixes it ;)

adammockor commented 8 years ago

I prepared gist. https://gist.github.com/adammockor/fbc2919e3fa4f4ba7ae62b09cda4e08f

sindresorhus commented 8 years ago

@adammockor Can you include it in this PR? I'd like to be able to run it on any new changes.

sindresorhus commented 8 years ago

ping :)

adammockor commented 8 years ago

Sorry for the delay. I include heapdump test as you wished, but io.js test is falling due to compatibility issues with heapdump module. Should I exclude heapdump from devDependecies?

sindresorhus commented 7 years ago

but io.js test is falling due to compatibility issues with heapdump module

Should be fixed in master. Rebase.

Seems you forgot to actually include the file.

sindresorhus commented 7 years ago

You also need to fix the merge conflict.

sindresorhus commented 7 years ago

Thank you @adammockor :)

adammockor commented 7 years ago

Thank you for your time and patience.