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

src: call uv_library_shutdown before DisposePlatform #45226

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Oct 28, 2022

When the process exits, the tasks in the thread pool may also need to access the data of platform object, such as trace agent, or a field added in the future. So make sure the thread pool exits first.

Refs: #44458

  • 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

@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. labels Oct 28, 2022
@tniessen
Copy link
Member

cc @nodejs/libuv

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but consider adding a comment to DisposePlatform() in src/node_v8_platform-inl.h that it's absolutely forbidden to directly or indirectly call into libuv.

(It doesn't look like it does but I didn't check in depth.)

When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see nodejs#44458
@theanarkh theanarkh force-pushed the DisposePlatform_after_uv_library_shutdown branch from 32a1961 to 2131b5a Compare October 29, 2022 15:39
@theanarkh
Copy link
Contributor Author

LGTM but consider adding a comment to DisposePlatform() in src/node_v8_platform-inl.h that it's absolutely forbidden to directly or indirectly call into libuv.

(It doesn't look like it does but I didn't check in depth.)

Done !

@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2022
@nodejs-github-bot
Copy link
Collaborator

@juanarbol juanarbol added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2022
juanarbol pushed a commit that referenced this pull request Oct 30, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@juanarbol
Copy link
Member

Landed in 08950fc

@juanarbol juanarbol closed this Oct 30, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants