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

domain: fix unintentional deprecation warning #34245

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 7, 2020

646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were not being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)
Fixes: #34069

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: nodejs@646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: nodejs@646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: nodejs#33801 (comment)
@addaleax addaleax requested a review from Qard July 7, 2020 16:25
@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 7, 2020
@addaleax addaleax added the domain Issues and PRs related to the domain subsystem. label Jul 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member

Also closes #34069

@addaleax
Copy link
Member Author

addaleax commented Jul 7, 2020

@codebytere Thanks, updated! :)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 7, 2020
@addaleax
Copy link
Member Author

addaleax commented Jul 7, 2020

👍 to fast-track? The change here is relatively straightforward.

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Jul 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@Qard
Copy link
Member

Qard commented Jul 7, 2020

Sorry, thanks for catching this. :)

@addaleax
Copy link
Member Author

addaleax commented Jul 7, 2020

Landed in 2243d79, thanks for the quick reviews!

@addaleax addaleax closed this Jul 7, 2020
addaleax added a commit that referenced this pull request Jul 7, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@addaleax addaleax deleted the domain-async-id-fixup branch July 7, 2020 20:08
@Flarna
Copy link
Member

Flarna commented Jul 11, 2020

I think we should get this one into v12.x-staging soon as #33801 has been included there.

@Flarna Flarna mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 14, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
codebytere pushed a commit to codebytere/node that referenced this pull request Jul 21, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: nodejs@646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: nodejs@646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: nodejs#33801 (comment)

PR-URL: nodejs#34245
Fixes: nodejs#34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. domain Issues and PRs related to the domain subsystem. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEP0097 warning triggered in REPL.
9 participants