Skip to content

lib: fix memory leak, when module require error occurs - #32837

Closed
lianxuify wants to merge 1 commit into
nodejs:masterfrom
lianxuify:module_memory_leak
Closed

lib: fix memory leak, when module require error occurs#32837
lianxuify wants to merge 1 commit into
nodejs:masterfrom
lianxuify:module_memory_leak

Conversation

@lianxuify

@lianxuifylianxuify commented Apr 14, 2020

Copy link
Copy Markdown
Contributor

Delete useless module in parent module: parent.children array when error occuers
so that the memory can be garbage collected.

Fixes: #32836

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

lianxuify
lianxuify commented Apr 14, 2020
View reviewed changes
Comment threadtest/sequential/test-module-loading.js Outdated

@lianxuifylianxuifyApr 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test for module loading, verify module.children contains all the different modules that we've required, but should not contain modules that failed to load

bnoordhuis
bnoordhuis approved these changes Apr 14, 2020
View reviewed changes
Comment threadlib/internal/modules/cjs/loader.js Outdated
@lianxuify
lib: fix memory leak, when module require error occurs
d0b97d7
Delete useless module in parent module: parent.children array when error occuers
so that the memory can be garbage collected.
Fixes: nodejs#32836
bnoordhuis
bnoordhuis approved these changes Apr 15, 2020
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM. I'll kick off the CI.

@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30750/

puzpuzpuz
puzpuzpuz approved these changes Apr 15, 2020
View reviewed changes

@puzpuzpuzpuzpuzpuz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for noticing and fixing this issue!

@puzpuzpuzpuzpuzpuz added module Issues and PRs related to the module subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 16, 2020
puzpuzpuz pushed a commit that referenced this pull request Apr 17, 2020
@lianxuify@puzpuzpuz
module: fix memory leak when require error occurs
6bcf968
Delete useless module in parent module: parent.children array
when error occurs, so that it can be garbage collected.
Fixes: #32836
PR-URL: #32837
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@puzpuzpuz

puzpuzpuz commented Apr 17, 2020

Copy link
Copy Markdown
Member

Landed in 6bcf968

I have slightly modified the prefix and detailed explanation in the commit message to make it more obvious. Hope you don't mind.

Once again, thanks for noticing and fixing this issue!

MylesBorins pushed a commit that referenced this pull request Apr 17, 2020
@lianxuify@MylesBorins
module: fix memory leak when require error occurs
fe1bda9
Delete useless module in parent module: parent.children array
when error occurs, so that it can be garbage collected.
Fixes: #32836
PR-URL: #32837
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@lianxuify

lianxuify commented Apr 18, 2020

Copy link
Copy Markdown
ContributorAuthor

Landed in 6bcf968

I have slightly modified the prefix and detailed explanation in the commit message to make it more obvious. Hope you don't mind.

Once again, thanks for noticing and fixing this issue!

Thank you for your modification, I learned.
I'm very happy that I can contribute to Nodejs！

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@lianxuify@targos
module: fix memory leak when require error occurs
cfb2998
Delete useless module in parent module: parent.children array
when error occurs, so that it can be garbage collected.
Fixes: nodejs#32836
PR-URL: nodejs#32837
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@lianxuify@BridgeAR
module: fix memory leak when require error occurs
520347c
Delete useless module in parent module: parent.children array
when error occurs, so that it can be garbage collected.
Fixes: #32836
PR-URL: #32837
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@BridgeARBridgeAR mentioned this pull request Apr 28, 2020
Merged
targos pushed a commit that referenced this pull request Apr 28, 2020
@lianxuify@targos
module: fix memory leak when require error occurs
6489a5b
Delete useless module in parent module: parent.children array
when error occurs, so that it can be garbage collected.
Fixes: #32836
PR-URL: #32837
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
+1 more reviewer
@puzpuzpuzpuzpuzpuzpuzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.moduleIssues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

memory leak, when module require error occurs

4 participants

@lianxuify@nodejs-github-bot@puzpuzpuz@bnoordhuis