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: deprecate legacy node::MakeCallback #18632

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Feb 8, 2018

The legacy MakeCallback functions do not provide a mechanism to
propagate async context. This means that any native modules using these
directly is likely breaking async debugging & tracing tools. For
example it is possible that such a module will cause incorrect async
stack traces to be reported (even when the module is not on the stack).

We've had context preserving versions of MakeCallback available since Node 8.x.

Ref: #13254, specifically #13254 (comment).
Related: nodejs/nan#729

This doesn't fit into any of the categories of deprecations as defined in our Deprecation policy. This is an API available to native modules via node.h. It is not otherwise documented. Chances are high that any given native module would be using this API. The migration path is fairly easy though.

We will now give a compile time warning when a module using the deprecated API is being compiled against Node 10. There is no runtime impact at this point. From this point of view, I guess this is closest to a Documentation-only deprecation.

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

src, async_hooks

/cc @nodejs/async_hooks @nodejs/addon-api @nodejs/diagnostics

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

@ofrobots ofrobots added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 8, 2018
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 8, 2018
Type: Compile-time

Certain versions of `node::MakeCallback` APIs available to native modules are
deprecated. Please use the versions of the API that accept a `async_context`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: a `async_context` -> an `async_context`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 8, 2018

As a semver-major this needs two @nodejs/tsc approvals.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2018
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM. Although, I think we need to declare C++ AsyncHooks Embedder API stable before we can do this. But I'm okay with that.

@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 9, 2018

@AndreasMadsen

Although, I think we need to declare C++ AsyncHooks Embedder API stable before we can do this. But I'm okay with that.

This would have been an ideal way of doing things, but it should not be a requirement IMHO:

  • We would want to have some feedback on the C++ AsyncHooks Embedder API before we move it out of experimental. It has been available since Node 8.x, and approximately 0% of modules are using it.
  • This is a compile-time warning only. People are not prevented from using the deprecated functions. It does encourage modules to switch to the C++ API, and that will drive feedback for the AsyncHooks embedder API.

IOW, we need this change if we want to move the C++ AsynchHooks Embedder API to move out of experimental.

@AndreasMadsen
Copy link
Member

@ofrobots Yeah, there is definitely conflicting interests. I think this will be for the TSC to decide.

The issue with deprecating it before the C++ Embedder API is marked stable, is that there will be no API for calling a callback that isn't either deprecated or experimental.

@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 9, 2018

@AndreasMadsen there are prior examples of that. For example, Domains are deprecated even though there is no stable alternative to them (still).

there will be no API for calling a callback that isn't either deprecated or experimental.

To be pedantic, MakeCallback is not experimental. It is always possible for people to pass in a nullary async_context.

@addaleax
Copy link
Member

addaleax commented Feb 9, 2018

This is a compile-time warning only. People are not prevented from using the deprecated functions. It does encourage modules to switch to the C++ API, and that will drive feedback for the AsyncHooks embedder API.

Maybe we could just set a time frame for the actual removal that is far enough in the future, or even better, indicate that we will never actually remove these functions?

@BridgeAR
Copy link
Member

Landed in a570aca 🎉

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
The legacy MakeCallback functions do not provide a mechanism to
propagate async context. This means that any native modules using these
directly is likely breaking async debugging & tracing tools. For
examples it is possible that such a module will cause incorrect async
stack traces to be reported (even when the module is not on the stack).

The new MakeCallback allow the user to specify the async context in
which the callback is to be executed.

Ref: nodejs#13254

PR-URL: nodejs#18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR closed this Feb 10, 2018
@apapirovski
Copy link
Member

Should this really have landed with a tsc-review label on it? 😕

@BridgeAR
Copy link
Member

Uh, gosh... I missed that... Hm, I am going to force push it out again as it is only a few minutes ago.

@BridgeAR BridgeAR reopened this Feb 10, 2018
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018
@ofrobots
Copy link
Contributor Author

@nodejs/tsc, this has the tsc-review label so please review.

Specifically @AndreasMadsen added the label around this question: #18632 (comment). He has LGTM'd this PR, so there isn't a conflict here, I think the intent is to make sure that the TSC is aware that this is happening. @AndreasMadsen please correct me if I misinterpreted your intent here. There are already the requisite 2 TSC LGTMs on this PR required for semver majors.

This is time sensitive, and should get into Node 10. As a semver major, it would be good to get this in before March. I am OOO starting next week, so I would like to make progress on this this week.

@apapirovski
Copy link
Member

Perhaps something that could be discussed at the Diagnostics Summit even, given that @AndreasMadsen is in attendance, I believe.

@ofrobots
Copy link
Contributor Author

@apapirovski We already have an LGTM from @AndreasMadsen. There is nothing to discuss. Unless if some questions/concerns come from the TSC as a result of the tsc-review label.

@MylesBorins
Copy link
Contributor

I think discussing how this API will be handled long term doesn't require blocking this deprecation landing if there is already consensus on doing so.

Unless someone has an explicit -1 I feel like perhaps this should land and an issue to discuss long term deprecation should be opened.

@ofrobots
Copy link
Contributor Author

Removing tsc-review as the tsc has been adequately pinged. I will land this later today.

@ofrobots ofrobots added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed tsc-review labels Feb 15, 2018
@sam-github
Copy link
Contributor

Unlike a docs-only deprecation, when native addons are installed, they will start printing compiler warnings about deprecated APIs to the console, where the installer of the addon (who is probably only installing it indirectly via a npm dep, and can't do anything about this) will see them, right? This strikes me as slightly more similar to a run-time deprecation that a doc deprecation.

I agree the problematic functions should be completely dropped ASAP so context propagation is preserved.

@ofrobots
Copy link
Contributor Author

@sam-github You're right. I guess it is somewhere in the middle. Compile warnings show up at npm install time, for modules that don't ship pre-compiled blobs. A runtime deprecation, on the other hand, would show up each time the app is run.

The legacy MakeCallback functions do not provide a mechanism to
propagate async context. This means that any native modules using these
directly is likely breaking async debugging & tracing tools. For
example it is possible that such a module will cause incorrect async
stack traces to be reported (even when the module is not on the stack).

The new MakeCallback allow the user to specify the async context in
which the callback is to be executed.

Ref: nodejs#13254
PR-URL: nodejs#18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@ofrobots ofrobots merged commit efb3259 into nodejs:master Feb 16, 2018
@ofrobots
Copy link
Contributor Author

Landed as efb3259.

@ofrobots ofrobots deleted the deprecate-old-make-callback branch February 16, 2018 02:35
ofrobots added a commit to ofrobots/node that referenced this pull request Feb 16, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: nodejs#18632
@ofrobots ofrobots mentioned this pull request Feb 16, 2018
2 tasks
jasnell pushed a commit that referenced this pull request Feb 16, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: #18632

PR-URL: #18810
Refs: #18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: #18632

PR-URL: #18810
Refs: #18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: #18632

PR-URL: #18810
Refs: #18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: #18632

PR-URL: #18810
Refs: #18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The legacy MakeCallback functions do not provide a mechanism to
propagate async context. This means that any native modules using these
directly is likely breaking async debugging & tracing tools. For
example it is possible that such a module will cause incorrect async
stack traces to be reported (even when the module is not on the stack).

The new MakeCallback allow the user to specify the async context in
which the callback is to be executed.

Ref: nodejs#13254
PR-URL: nodejs#18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The legacy MakeCallback deprecation was resulting in compile time
warnings in adddon tests. Fix them.

Ref: nodejs#18632

PR-URL: nodejs#18810
Refs: nodejs#18632
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
mcollina added a commit to mcollina/node that referenced this pull request Mar 15, 2019
AsyncResource.emitBefore and AsyncResource.emitAfter have been
deprecated in nodejs#18632. This PR removes
it all.
This commit also updates some embedder tests to use internal APIs.
The conditions are still possible for Node.js core developers but not
for end users.
mcollina added a commit that referenced this pull request Mar 18, 2019
AsyncResource.emitBefore and AsyncResource.emitAfter have been
deprecated in #18632. This PR removes
it all.
This commit also updates some embedder tests to use internal APIs.
The conditions are still possible for Node.js core developers but not
for end users.

PR-URL: #26530
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: James M Snell <jasnell@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++. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.