src: add context-aware helpers and init macro #21318

gabrielschulhof · 2018-06-13T18:09:41Z

Introduces macro NODE_MODULE_INIT() which creates the boilerplate for
a context-aware module which can be loaded multiple times via the
special symbol mechanism.

Also introduces class node::WeakData with which one can weakly
associate a native class instance with a JavaScript object, and
convenience methods node::SetMethodWithData() and
node::SetPrototypeMethodWithData() which pass a native pointer
through to bindings.

Together, these APIs allow one to avoid using global static data in
an addon.

Re: #21291 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-06-13T18:09:42Z

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

addaleax · 2018-06-13T22:17:22Z

src/node.h

+
+  recv->PrototypeTemplate()->Set(fn_name, t);
+}
+


I wouldn’t be a fan of including this in our public API, unless there’s a reason why we would export it from core

We originally had more core of this sort around (e.g. node_object_wrap.h), but it’s a good idea that this generally moved into Nan territory? I’d suggest PR’ing it there

vsemozhetbyt · 2018-06-15T20:36:14Z

doc/api/addons.md

@@ -98,6 +98,107 @@ the `.node` suffix).
 In the `hello.cc` example, then, the initialization function is `Initialize`
 and the addon module name is `addon`.

+When building your addon with node-gyp, using the macro `NODE_GYP_MODULE_NAME`


node-gyp -> `node-gyp` for consistency with other mentions?

vsemozhetbyt · 2018-06-15T20:42:29Z

doc/api/addons.md

+invocation of `NODE_MODULE_INIT()`:
+* `Local<Object> exports`,
+* `Local<Value> module`, and
+* `Local<Context> context


Missing final backtick and period?

vsemozhetbyt · 2018-06-15T20:48:09Z

doc/api/addons.md

+* Constructing an instance of this class in the addon initializer such that
+`exports` is passed to the constructor,
+* Storing the instance in a `v8::External`, and
+* exposing methods to JavaScript via `node::SetMethodWithData()` and via


Should the first letter case be unified in this list?

gabrielschulhof · 2018-06-20T22:31:40Z

@addaleax @bnoordhuis I have reduced the scope of the PR to the NODE_MODULE_INIT() macro, documentation pertaining to context-aware modules, and an example illustrating its use in a situation where data is stored per addon instance.

gabrielschulhof · 2018-06-20T22:32:07Z

@vsemozhetbyt I have fixed the issues you mention.

vsemozhetbyt · 2018-06-20T22:37:19Z

Doc format LGTM.

jasnell · 2018-06-21T16:10:15Z

doc/api/addons.md

@@ -98,6 +98,120 @@ the `.node` suffix).
 In the `hello.cc` example, then, the initialization function is `Initialize`
 and the addon module name is `addon`.

+When building your addon with `node-gyp`, using the macro `NODE_GYP_MODULE_NAME`


nit: "When building addons with..."

Yep, I let that one slip :) Thanks!

gabrielschulhof · 2018-06-22T20:35:59Z

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

gabrielschulhof · 2018-06-26T22:28:05Z

Another CI before I land: https://ci.nodejs.org/job/node-test-pull-request/15639/

devsnek · 2018-06-26T22:30:47Z

this needs another approval to land

gabrielschulhof · 2018-06-26T22:41:35Z

@devsnek aaah, thank you! Wasn't sure.

vsemozhetbyt · 2018-06-27T05:13:37Z

It seems our docs state only one approving is needed to land a not fast-track PR.

gabrielschulhof · 2018-06-27T13:40:27Z

@vsemozhetbyt actually, it says it must be two if a Collaborator is the originator of the PR. I checked after @devsnek's reminder: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#code-reviews

vsemozhetbyt · 2018-06-27T14:26:58Z

Do you refer this sentence?

In the case of pull requests proposed by an existing Collaborator, an additional Collaborator is required for sign-off.

I read it as "One Collaborator is the PR author, another Collaborator is a reviewer".

gabrielschulhof · 2018-06-27T15:11:25Z

I don't read it that way, because I know that the PR's author may not approve. Therefore, since all PRs need at least one approver, and a Collaborator's PR needs and additional approver, I conclude that a Collaborator's PR needs to approvers. Still, it should maybe state explicitly that a Collaborator's PR needs the approver of two Collaborators.

…

On Wed, Jun 27, 2018 at 10:29 AM, Vse Mozhet Byt ***@***.***> wrote: Do you refer this sentence? In the case of pull requests proposed by an existing Collaborator, an additional Collaborator is required for sign-off. I read it as "One Collaborator is the PR author, another Collaborator is a reviewer". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21318 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0YUDmU2AqXPuk33Qw_GV-exV3InLks5uA5awgaJpZM4UmriR> .

vsemozhetbyt · 2018-06-27T15:17:20Z

Let's see how others understand this. It seems we have a bit confusing wording here.

Trott · 2018-06-28T17:22:09Z

This does not need a second reviewer, although I would try pinging a relevant team or two before landing, just to get a second one.

gabrielschulhof · 2018-06-28T17:25:01Z

@nodejs/collaborators in the spirit of #21565 (comment), could somebody else please have a look? 🙂

ryzokuken · 2018-06-28T17:27:54Z

Aww, the dreaded mention.

P.S. I'll take a look :)

mcollina

LGTM

mcollina · 2018-06-28T17:56:53Z

doc/api/addons.md

+### Context-aware addons
+
+There are environments in which Node.js addons may need to be loaded multiple
+times in multiple contexts. The macro `NODE_MODULE_INIT()` will construct an


Can you please articulate what could be those cases?

@mcollina how about the new paragraph I added?

That’s perfect!

ryzokuken

LGTM, but if I were you, I'd wait for @addaleax's review.

gabrielschulhof · 2018-06-29T16:28:32Z

@addaleax could you please take another look?

Introduces macro `NODE_MODULE_INIT()` which creates the boilerplate for a context-aware module which can be loaded multiple times via the special symbol mechanism. Additionally, provides an example of using the new macro to construct an addon which stores per-addon-instance data in a heap-allocated structure that gets passed to each binding, rather than in a collection of global static variables. Re: nodejs#21291 (comment)

gabrielschulhof · 2018-07-01T01:53:09Z

Rebased and added doc for NODE_MODULE_INITIALIZER.

Trott · 2018-07-01T03:21:33Z

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

gabrielschulhof · 2018-07-03T21:10:53Z

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

gabrielschulhof · 2018-07-03T22:35:13Z

Landed in 602da64.

Introduces macros `NODE_MODULE_INITIALIZER` which expands to the name of the special symbol that process.dlopen() will look for to initialize an addon, and `NODE_MODULE_INIT()` which creates the boilerplate for a context-aware module which can be loaded multiple times via the special symbol mechanism. Additionally, provides an example of using the new macro to construct an addon which stores per-addon-instance data in a heap-allocated structure that gets passed to each binding, rather than in a collection of global static variables. Re: nodejs#21291 (comment) PR-URL: nodejs#21318 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

Trott · 2018-07-03T22:53:13Z

CI was not green. I think we need stronger wording in our docs about not landing stuff when CI is not green. I'll open a PR for that right now.

If you have a mostly-green node-test-pull-request, you're fairly certain the not-green stuff isn't relevant to your changes, and you want to re-run just the things that aren't green, use "Resume Build" on the left menu. It will create a new node-test-pull-request, copy over all the green stuff, and only re-run the red/yellow/grey stuff.

~~Post-landing CI: https://ci.nodejs.org/job/node-test-pull-request/15717/~~

EDIT: Can't re-run CI because the branch has been deleted. So in lieu of that, here's a node-daily-master run manually kicked off to re-test this: https://ci.nodejs.org/job/node-daily-master/1203/

gabrielschulhof · 2018-07-03T23:00:05Z

@Trott sorry about that! One job failed on what looked like an infra issue (unable to create a directory). Using "Resume Build" is an awesome option! I didn't know about it.

Trott · 2018-07-03T23:09:30Z

@Trott sorry about that! One job failed on what looked like an infra issue (unable to create a directory). Using "Resume Build" is an awesome option! I didn't know about it.

Sorry, didn't mean to pick on you! No need to apologize. You're far from alone. I'm just trying to spread the word about Resume Build. :-D (I didn't know about it until someone else on Build showed it to me...I think it was @refack.)

Introduces macros `NODE_MODULE_INITIALIZER` which expands to the name of the special symbol that process.dlopen() will look for to initialize an addon, and `NODE_MODULE_INIT()` which creates the boilerplate for a context-aware module which can be loaded multiple times via the special symbol mechanism. Additionally, provides an example of using the new macro to construct an addon which stores per-addon-instance data in a heap-allocated structure that gets passed to each binding, rather than in a collection of global static variables. Re: #21291 (comment) PR-URL: #21318 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 13, 2018

gabrielschulhof requested review from bnoordhuis and addaleax June 13, 2018 18:09

gabrielschulhof force-pushed the node-addon-init branch 2 times, most recently from e292f6d to f25f30b Compare June 13, 2018 18:29

addaleax reviewed Jun 13, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 15, 2018

View reviewed changes

gabrielschulhof force-pushed the node-addon-init branch from e83beeb to 0f7d65d Compare June 20, 2018 22:28

gabrielschulhof force-pushed the node-addon-init branch 2 times, most recently from 237c4f3 to ac20ee3 Compare June 20, 2018 22:40

jasnell reviewed Jun 21, 2018

View reviewed changes

gabrielschulhof force-pushed the node-addon-init branch from ac20ee3 to 49360b2 Compare June 21, 2018 17:33

jasnell approved these changes Jun 22, 2018

View reviewed changes

gabrielschulhof force-pushed the node-addon-init branch from 49360b2 to 6b68df1 Compare June 22, 2018 20:34

vsemozhetbyt mentioned this pull request Jun 27, 2018

meta: number of approval ambiguity #21565

Closed

mcollina approved these changes Jun 28, 2018

View reviewed changes

ryzokuken approved these changes Jun 28, 2018

View reviewed changes

Gabriel Schulhof added 3 commits June 30, 2018 20:24

address review comments

a9d8063

document NODE_MODULE_INITIALIZER

9402ca1

gabrielschulhof force-pushed the node-addon-init branch from f81217c to 9402ca1 Compare July 1, 2018 01:48

gabrielschulhof closed this Jul 3, 2018

gabrielschulhof deleted the node-addon-init branch July 3, 2018 22:35

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

src: add context-aware helpers and init macro #21318

src: add context-aware helpers and init macro #21318

Conversation

gabrielschulhof commented Jun 13, 2018

Checklist

nodejs-github-bot commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 20, 2018

gabrielschulhof commented Jun 20, 2018

vsemozhetbyt commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 22, 2018

gabrielschulhof commented Jun 26, 2018

devsnek commented Jun 26, 2018

gabrielschulhof commented Jun 26, 2018

vsemozhetbyt commented Jun 27, 2018

gabrielschulhof commented Jun 27, 2018

vsemozhetbyt commented Jun 27, 2018 • edited Loading

gabrielschulhof commented Jun 27, 2018 via email

vsemozhetbyt commented Jun 27, 2018 • edited Loading

Trott commented Jun 28, 2018

gabrielschulhof commented Jun 28, 2018

ryzokuken commented Jun 28, 2018

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 29, 2018

gabrielschulhof commented Jul 1, 2018

Trott commented Jul 1, 2018

gabrielschulhof commented Jul 3, 2018

gabrielschulhof commented Jul 3, 2018

Trott commented Jul 3, 2018 • edited Loading

gabrielschulhof commented Jul 3, 2018

Trott commented Jul 3, 2018 • edited Loading

vsemozhetbyt commented Jun 27, 2018 •

edited

Loading

vsemozhetbyt commented Jun 27, 2018 •

edited

Loading

Trott commented Jul 3, 2018 •

edited

Loading

Trott commented Jul 3, 2018 •

edited

Loading