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

Jump to bottom

src: call uv_library_shutdown before DisposePlatform #45226

Closed
wants to merge 1 commit into from
Closed

src: call uv_library_shutdown before DisposePlatform #45226

wants to merge 1 commit into from

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Oct 28, 2022 •
edited

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

bnoordhuis
bnoordhuis approved these changes Oct 29, 2022
View reviewed changes
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.)

@theanarkh
src: call uv_library_shutdown before DisposePlatform
2131b5a 
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

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

@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
@theanarkh @juanarbol
src: call uv_library_shutdown before DisposePlatform
08950fc 
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
@theanarkh @RafaelGSS
src: call uv_library_shutdown before DisposePlatform
4a31d2a 
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
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@theanarkh @RafaelGSS
src: call uv_library_shutdown before DisposePlatform
42db849 
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
@theanarkh @danielleadams
src: call uv_library_shutdown before DisposePlatform
61736da 
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 danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@theanarkh @danielleadams
src: call uv_library_shutdown before DisposePlatform
1a437c3 
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
@theanarkh @danielleadams
src: call uv_library_shutdown before DisposePlatform
8c4ac6d 
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>
@santigimeno santigimeno mentioned this pull request Jan 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@santigimeno santigimeno santigimeno approved these changes

@juanarbol juanarbol juanarbol approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@theanarkh @tniessen @nodejs-github-bot @juanarbol @bnoordhuis @santigimeno