process: use owner symbol to access more public APIs in _getActive* by addaleax · Pull Request #22002 · nodejs/node

addaleax · 2018-07-27T12:43:37Z

Use a standardized owner_symbol key to access the corresponding “public” JS wrapper object for a native handle. Currently, this is only implemented for the objects that already had some sort of owner keys, but this should be a first step towards making this a more common pattern.

/cc @nodejs/diagnostics

Checklist

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

nodejs-github-bot · 2018-07-27T12:43:39Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/367/pipeline

jasnell

Nice.

mcollina

LGTM

maclover7 · 2018-07-27T14:49:50Z

I forget if there's a deprecation process for to remove strings from Environment, but looks like the last two use cases for env->owner_string() are removed here, so that could possibly be ✂️as well

addaleax · 2018-07-27T14:55:07Z

@maclover7 env.h has NODE_WANT_INTERNALS guards, it’s private API and we don’t need to worry about that. I’ve removed them, thanks for catching that 🙂

refack · 2018-07-27T15:16:16Z

src/async_wrap.cc

What will happen if we have a pathological case of cyclic ownership?

a[owner_symbol] = b; b[owner_symbol] = a;

Is there a cycle safe STL algorithm for such a recursive walk?

It’s going to run into an infinite loop, yes.

Is there a cycle safe STL algorithm for such a recursive walk?

No, but you could implement something like a set of seen objects. The performance impact would be very noticeable, though.

You could just limit the loop to an arbitrary number of iterations and crash. IMHO better than a infinite busy loop, but I have no strong feelings about this.

AndreasMadsen · 2018-07-27T17:43:58Z

This is awesome, does this mean we can also start exposing the owner resource as the async_hooks resource?

jasnell · 2018-07-27T17:46:16Z

This is awesome, does this mean we can also start exposing the owner resource as the async_hooks resource?

Certainly safer to do so. I still think it may be worthwhile exploring a minimal shared interface for such resources, but I'd be much happier exposing the owner than the handle.

addaleax · 2018-07-27T18:33:15Z

@AndreasMadsen @jasnell I’d really like that (and the fact that GetOwner() methods are being introduced here are a step towards it), but it’s not happening in this PR at least because the native handles are constructed before the property can be set on them. I think it’s doable but not trivial to make that work.

BridgeAR · 2018-07-29T02:07:11Z

This needs a rebase.

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role.

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`.

addaleax · 2018-07-29T14:48:04Z

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

Fishrock123 · 2018-07-30T16:00:14Z

This seems like a good idea.

Two questions:

Are we sure we've covered backwards compat enough for now?
Should we deprecate the .owner get/setter, or is that waiting for some other time?

Fishrock123 · 2018-07-30T16:01:25Z

Hmm, on second thought... since this symbol isn't exposed, doesn't this make it difficult to get the owner from a handle as available from an async hook callback?

AndreasMadsen · 2018-07-30T20:58:16Z

@Fishrock123 Just want to point out that the resource in async_hooks is not a public API. Especially since running console.log(resource) can crash the application ;)

The API for accessing this information is currently not considered public, but using the Embedder API, users can provide and document their own resource objects.

Fishrock123 · 2018-07-30T21:49:25Z

Just want to point out that the resource in async_hooks is not a public API.

That doesn't make it not a thing to think about.

Especially since running console.log(resource) can crash the application ;)

I can think of lots of ways to crash a node application. Yes, accessing a resource during the init callback specifically is sometimes one of them.

That doesn't mean that people aren't using it, or that it is useful, or that there aren't use-cases we should still keep in mind even for "unofficial" things.

addaleax · 2018-07-30T22:04:48Z

Are we sure we've covered backwards compat enough for now?

I’m not sure what more to provide than getter/setter pairs for the deprecated properties.

The more breaking-change-y patches will be when we add this symbol to other types of handles.

Should we deprecate the .owner get/setter, or is that waiting for some other time?

No hurry, I’d say.

Hmm, on second thought... since this symbol isn't exposed, doesn't this make it difficult to get the owner from a handle as available from an async hook callback?

@Fishrock123 Would that concern be alleviated once we move async_hooks to also use this symbol for resource objects?

addaleax · 2018-08-01T16:13:29Z

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

Edit: Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16127/

Edit: Resume Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16147/

addaleax · 2018-08-05T12:00:24Z

Landed in af7164e, 5be9b1d

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. PR-URL: #22002 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. PR-URL: #22002 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

targos · 2018-08-06T19:25:42Z

Should this be backported to v10.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.

Backport should be relatively easy. There is one more instance of owner_string in src/timer_wrap.cc.

targos · 2018-08-19T19:03:52Z

Ping @addaleax

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. PR-URL: nodejs#22002 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. PR-URL: nodejs#22002 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`. PR-URL: #22002 Backport-PR-URL: #22507 Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 27, 2018

addaleax added the process Issues and PRs related to the process subsystem. label Jul 27, 2018

jasnell approved these changes Jul 27, 2018

View reviewed changes

cjihrig approved these changes Jul 27, 2018

View reviewed changes

maclover7 approved these changes Jul 27, 2018

View reviewed changes

mcollina approved these changes Jul 27, 2018

View reviewed changes

joyeecheung approved these changes Jul 27, 2018

View reviewed changes

devsnek approved these changes Jul 27, 2018

View reviewed changes

refack reviewed Jul 27, 2018

View reviewed changes

JungMinu approved these changes Jul 28, 2018

View reviewed changes

TimothyGu approved these changes Jul 28, 2018

View reviewed changes

AndreasMadsen mentioned this pull request Jul 28, 2018

async_hooks: add getActiveResources (prototype) #21453

Closed

4 tasks

addaleax added 4 commits July 29, 2018 16:27

lib,src: standardize owner_symbol for handles

df90330

Instead of somtimes using an `owner` string to link from a native handle object to the corresponding JS object, standardize on a single symbol that fulfills this role.

process: use owner_symbol for _getActive*

df15145

This makes it easier to provide public APIs in the return types of `process._getActiveHandles()` and `process._getActiveRequests()`.

fixup! process: use owner_symbol for _getActive*

4c477b9

fixup! lib,src: standardize owner_symbol for handles

8d547e5

addaleax force-pushed the async-owner branch from 6167376 to 8d547e5 Compare July 29, 2018 14:47

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 29, 2018

ofrobots approved these changes Aug 1, 2018

View reviewed changes

addaleax closed this Aug 5, 2018

addaleax deleted the async-owner branch August 5, 2018 12:00

targos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 6, 2018

addaleax mentioned this pull request Aug 24, 2018

[v10.x] backport owner_symbol for process._getActive* #22507

Closed

targos added backported-to-v10.x and removed backport-requested-v10.x labels Aug 28, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

Uh oh!

Conversation

addaleax commented Jul 27, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jul 27, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Jul 27, 2018

Uh oh!

addaleax commented Jul 27, 2018

Uh oh!

refack Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

refack Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 27, 2018

Uh oh!

jasnell commented Jul 27, 2018

Uh oh!

addaleax commented Jul 27, 2018

Uh oh!

BridgeAR commented Jul 29, 2018

Uh oh!

addaleax commented Jul 29, 2018

Uh oh!

Fishrock123 commented Jul 30, 2018

Uh oh!

Fishrock123 commented Jul 30, 2018

Uh oh!

AndreasMadsen commented Jul 30, 2018

Uh oh!

Fishrock123 commented Jul 30, 2018

Uh oh!

addaleax commented Jul 30, 2018

Uh oh!

addaleax commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Aug 5, 2018

Uh oh!

targos commented Aug 6, 2018

Uh oh!

targos commented Aug 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

addaleax commented Aug 1, 2018 •

edited

Loading