Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

worker: fix heap snapshot crash on exit #43123

Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 17, 2022

Worker.parent_port_ can be released before the exit event of Worker. The parent_port_ is not owned by node::worker::Worker, thus the reference to it is not always valid, and accessing it at exit crashes the
process.

As the Worker.parent_port_ is only used in the memory info tracking, and the corresponding MessagePort
object is already been tracking with the Worker.messagePort javascript property , it can be safely removed.

Fixes: #43122

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels May 17, 2022
@legendecas legendecas changed the title src: remove unnecessary Worker.parent_port_ tracking worker: remove unnecessary Worker.parent_port_ tracking May 17, 2022
@legendecas legendecas marked this pull request as ready for review May 17, 2022 04:42
@legendecas legendecas force-pushed the worker-exit-heap-snapshot branch 3 times, most recently from 425642e to 21fedfe Compare May 17, 2022 04:53
@legendecas legendecas changed the title worker: remove unnecessary Worker.parent_port_ tracking worker: fix heap snapshot crash on exit May 17, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

Landed in 448d5d1

@legendecas legendecas closed this May 19, 2022
legendecas added a commit that referenced this pull request May 19, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@legendecas legendecas deleted the worker-exit-heap-snapshot branch May 19, 2022 14:00
bengl pushed a commit that referenced this pull request May 30, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@bengl bengl mentioned this pull request May 31, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: #43123
Fixes: #43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: nodejs/node#43123
Fixes: nodejs/node#43122
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heap snapshot crash when using worker_threads
4 participants