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: distinguish refed/unrefed threadsafe Immediates #33320

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 9, 2020

In some situations, it can be useful to use threadsafe callbacks
on an Environment to perform cleanup operations that should run
even when the process would otherwise be ending.

(Split out from #33321)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 9, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 9, 2020

@@ -727,9 +727,9 @@ void Environment::SetUnrefImmediate(Fn&& cb) {
}

template <typename Fn>
void Environment::SetImmediateThreadsafe(Fn&& cb) {
void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) {
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer an enum rather than a magical boolean :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell Hm yeah … this reaches into the CallbackQueue implementation which also takes this as a boolean argument and returns the result as a boolean, so maybe in a follow-up PR? I’d like to be consistent here…

@addaleax addaleax added review wanted PRs that need reviews. lts-watch-v12.x labels May 13, 2020
addaleax added a commit that referenced this pull request May 16, 2020
In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.

PR-URL: #33320
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

Landed in e346261

@addaleax addaleax closed this May 16, 2020
@addaleax addaleax deleted the env-setimmediate-refed branch May 16, 2020 10:14
codebytere pushed a commit that referenced this pull request May 16, 2020
In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.

PR-URL: #33320
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request May 17, 2020
@codebytere codebytere mentioned this pull request May 18, 2020
jasnell pushed a commit that referenced this pull request May 22, 2020
Refs: #33320 (comment)

PR-URL: #33444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.

PR-URL: #33320
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit to codebytere/node that referenced this pull request Jun 9, 2020
In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.

PR-URL: nodejs#33320
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 9, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
Refs: #33320 (comment)

PR-URL: #33444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit to addaleax/node that referenced this pull request Jul 8, 2020
Refs: nodejs#33320 (comment)

PR-URL: nodejs#33444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit that referenced this pull request Sep 23, 2020
Refs: #33320 (comment)

PR-URL: #33444
Backport-PR-URL: #34263
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins removed lts-watch-v12.x review wanted PRs that need reviews. labels Nov 16, 2020
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants