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: introduce internal C++ SetImmediate() and remove async_hooks destroy timer #17117

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Introduce env->SetImmediate(fn) and remove the handle we use for async_hooks destroys.

After #17105, a similar thing could be done for HTTP/2.

(The one test change is due to the slightly different timing for async_hooks, since timer and check handle callbacks run at different times.)

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

src

@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 Nov 18, 2017
@addaleax
Copy link
Member Author

addaleax commented Nov 18, 2017

@addaleax addaleax force-pushed the set-immediate-native branch 2 times, most recently from 6df5d66 to 88403c4 Compare November 18, 2017 19:26
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 19, 2017
@jasnell
Copy link
Member

jasnell commented Nov 20, 2017

i'm going to be revisiting the lifecycle of http2 sessions and streams soon after #17105 lands, good timing on this :-)

@addaleax addaleax mentioned this pull request Nov 21, 2017
4 tasks
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
@addaleax
Copy link
Member Author

Landed in 85f3e31, 5d7f5c1

@addaleax addaleax closed this Nov 21, 2017
@addaleax addaleax deleted the set-immediate-native branch November 21, 2017 11:50
addaleax added a commit that referenced this pull request Nov 21, 2017
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Nov 21, 2017
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@jasnell
Copy link
Member

jasnell commented Dec 11, 2017

Yes, this will need to be back ported as it is needed by http2 (or at least will be soon)

@MylesBorins
Copy link
Contributor

Managed to get it to land

MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 9, 2018

@addaleax @mcollina ... this needs a backport to v8.x-staging in order for me to be able to successfully backport the current http2 implementation to v8.x.

I'll try to get to it this week but if one of you happen to get to it first I'll owe you one beverage of your choice :-)

addaleax added a commit to addaleax/node that referenced this pull request Jan 10, 2018
addaleax added a commit to addaleax/node that referenced this pull request Jan 10, 2018
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
PR-URL: nodejs#17117
Backport-PR-URL: nodejs#18179
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
PR-URL: nodejs#17117
Backport-PR-URL: nodejs#18179
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
Backport-PR-URL: #18179
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
Backport-PR-URL: #18179
PR-URL: #17117
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

ping re: 8.x backport

Set don't land for v6.x

@addaleax
Copy link
Member Author

@MylesBorins The v8.x backport for this was in #18086 (or @AndreasMadsen’s #18179)

What’s the reason for dont-land-on-v6.x? I’d consider it possible that other backports in the future may want to have SetImmediate available.

@mcollina
Copy link
Member

@addaleax I'm for backporting on a need-to basis. This was backported to 8 because it was needed for http2. Given that we are not going to backport that, what's the need for v6? Of course, if there is something we want backported that depends on this, we should backport. I don't think there is a need to backport sooner.

@MylesBorins
Copy link
Contributor

I assumed it wouldn't land on 6 as there are not currently plans to backport async hooks. Feel to change labels as appropriate though

@addaleax
Copy link
Member Author

@MylesBorins The first commit here isn’t really about async_hooks, it introduces a very general internal utility – I’m okay with doing what @mcollina suggests and just backporting when it starts to get used by something else

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.

5 participants