Initial version of eps for ABI-Stable-Module-API

[mhdawson]

First cut of the ABI-Stable-Module-API eps. It is far from complete but is intended to allow insight into the current direction/status and will be updated regularly as we progress along the investigation and can make the API definition more complete.

[mhdawson]

FYI @nodejs/api, @nodejs/addon-api

[kkoopa]

Regarding what addons to test on, don't just go for the most popular ones, pick the most complex addons, in regard to the binding part, not whatever libraries they provide. NAN's test suite, while far from comprehensive, is a good start.

There is also need to decide on what to do with Isolates. The main reason that NAN does not currently do Isolates is because of 0.10-compatibility. Should Node ever be multi-isolate, from a practical point of view? I can only think of one or two addons I know of that actually use multiple Isolates.

[Fishrock123]

first reactions: doesn't seem quite as minimal as discussed?

Also, ni is an obscure and pretty uninformative namespace. :/ even api might be better?

[jasnell]

@Fishrock123

first reactions: doesn't seem quite as minimal as discussed?

How so? Seems to be exactly what was discussed.

[Fishrock123]

How so? Seems to be exactly what was discussed.

Perhaps it wasn't communicated fully?

I was the under the impression we'd do only:

• call into JS & back
• call into c++ & back
• convert to basic types

As such, things like function templates seem a little beyond that? Perhaps I am wrong.

[Fishrock123]

Additionally I don't think the EP process along will be enough to suffice for this. (i.e. we shouldn't just write up this conceptually and sign it off)

We should aim to not expand the EP from here directly but rather get these working in a shim, make sure that suits modules, get the feedback from that, and iterate. We should make sure to make it without any guarantees until we pass a finalized version in core though since versions of it are unlikely to be maintainable for long and the API may change.

I'm totally down to help where possible, I care about this an awful lot.

[kkoopa]

I'll also note that the best way of actually getting this new API into use would be to enable NAN (or a replacement) to add another (transitional) shim on top of this, ensuring backwards compatibility at least down to 0.12 or 4.0, which presumably will not have access to it. No addon maintainer wants to maintain several versions of the same code base.

[jasnell]

+1. But we'd have to call it naan then ;) On Apr 30, 2016 6:03 PM, "Benjamin Byholm" notifications@github.com wrote:

I'll also note that the best way of actually getting this new API into use would be to enable NAN (or a replacement) to add another (transitional) shim on top of this, ensuring backwards compatibility at least down to 0.12 or 4.0, which presumably will not have access to it. No addon maintainer wants to maintain several versions of the same code base.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/nodejs/node-eps/pull/20#issuecomment-216006021

[kkoopa]

One more thing, make it possible to seamlessly integrate this new (stable) API with the (unstable) VM-specific API. There is no realistic way of foreseeing every possible use case or developing a practical solution that will fit everything. What I consider one of the strong aspects of NAN is that the developer is free to pick and mix between raw V8 and NAN. Some things are not achievable in any other way, but requiring a couple of specific things here and there should not condemn the rest of the code to constant breaking. For example, promises are (currently) not handled by NAN, since they are not available in Node 0.10 and there is not realistic way of backporting all of that. However, a developer is still able to easily use promises where available, while the rest of the code can (safely) use NAN. C# has a similar concept in that mixing safe and unsafe code is quite doable.

[Fishrock123]

One more thing, make it possible to seamlessly integrate this new (stable) API with the (unstable) VM-specific API. There is no realistic way of foreseeing every possible use case or developing a practical solution that will fit everything.

I think the goal will probably end up making sure at very least node core can run ontop of it. (Since that's part of VM neutrality anyways.)

[jeisinger]

I'm a bit surprised by the direction this took. I was under the impression that we agreed on a different plan in the F2F meeting, namely to take the NAN module and replace the remaining v8 dependencies with new wrappers, i.e., v8::Localv8::Object should become something like nan::Localnan::Object.

The direction this is now taking seems to be a newly designed C-style API.

Deviating that much from NAN will make it unnecessarily difficult for native modules to transition over. A C-style API precludes any inlining based optimizations and puts additional cognitive burden on module authors that will have to explicitly manage lifetime of handles as opposed to relying on the Local dtor deregistering them etc..

Another thing we agreed upon was that this API should insulate native modules from the VM - using the API inside node as a means of becoming fully VM neutral was an explicit non-goal, as the latter will require fundamentally different APIs.

[jasnell]

+1 that the primary goal was to have an API that protects module developers from the internal VM changes. It's absolutely correct that providing a new internal abstraction was a non-goal for this initially -- if it happens to be usable in that way, then awesome, but if not, the focus needs to be on making sure module developers are protected.

As far as the relationship with nan is concerned, the idea was certainly to start with nan and iterate incrementally to begin abstracting the parts where V8's API bleeds through.

[mhdawson]

@jeisinger

1) When we looked at the functions in Nan, they were along these lines, while the unwrapped parts of v8 were more pure C++. So in extending the Nan methods this way seemed natural. I think the first deviation in what is there that was not already that way in Nan is:

node::ni::callMethod(ni_env,recv,,,)

as these others like: Nan::New() were already there.

The other areas would be object life cyle and exception handling, but would those be more than defining objects that the same methods could be called on ? For pushScope/popScope for lifecyle the answer is probably yes, the others I'm not so sure.

In terms of using templates etc, I think that is still possible in either case, and the in-lining needs to stop before any engine code gets pulled into the module.

Agreed, that this looks more C like than when we discussed at the meeting but I think that came out of the existing methods we pulled over from Nan. We can iterate to move in the right direction.

2) Agree that "using the API inside node as a means of becoming fully VM neutral was an explicit non-goal for now"

@kkoopa agreed that a Nan shim to allow this to be used transparently is one of the overall goals

@Fishrock123 agreed we can't just write this up and sign off. We just want to give some visibility along this way. This eps should stay in draft while we iterate and be update frequently

[jeisinger]

It might be helpful to take an existing module and convert it over to the proposed API (at least parts of it to demonstrate how the new API is supposed to be used, and to get a better feeling about the changes in ergonomics this would mean).

[mhdawson]

@jeisinger that is what I think the next step is as well. Write the code for a minimal set of the APIs and then show it working with a simple module to make things more concrete.

[mhdawson]

I'll let this settle for a day or more and then incorporate the "simple" updates (typos, clarifications, etc.)

[ianwjhalliday]

@kkoopa Which addons are those? Sounds like they are good to have on hand for investigation once we get the balling rolling on working prototype.

There is also need to decide on what to do with Isolates. The main reason that NAN does not currently do Isolates is because of 0.10-compatibility. Should Node ever be multi-isolate, from a practical point of view? I can only think of one or two addons I know of that actually use multiple Isolates.

[kkoopa]

webworker-threads

[Fishrock123]

Should Node ever be multi-isolate, from a practical point of view

Yes, so that workers are a thing we can do since we've been wanting to for a while but it has been too complex to review effectively. See: https://github.com/nodejs/node/pull/2133

[trevnorris]

Please replace <PRE> with three grave symbols. (Backticks)

[ianwjhalliday]

Updated document looks great

[ianwjhalliday]

:+1:

[mhdawson]

@nodejs/collaborators could I get a few reviews/L.G.T.M.s or comments. Discussion in last CTC meeting was that it can land as draft status even if we have not capture the final state of the API.

[saghul]

Had a quick read and left some comments, hope that helps.

[Qard]

One thing I'd like clarified is if the C++ API sits atop the C API, or vice versa.

If the C++ is a thin layer over the C, the C++ could perhaps be shipped as a header file, allowing the benefits of C++ API friendliness without dealing with the ABI stability concerns.

[ianwjhalliday]

@Qard that's correct, the intention is a C++ layer that is shipped entirely in a header and is built upon the C exports only, providing no exports of its own, for ABI stability.

Agree this should be clearly stated in the document.

@mhdawson perhaps we should mention this as a bullet in the Proposed Action section? We can also be explicit about this intent in the C++ Wrapper API section.

[Qard]

Great, thanks for the clarification! :smiley_cat:

[mhdawson]

Ok, plan to address the most recent comments and the hopefully get enough oks to land. Once landed additional changes can then be more easily addressed through pull requests.

[mhdawson]

I think this section in the doc:

However, we also believe that a C++ wrapper is possible and would provide ease of use. The key element is that the C++ part must all be inlined and only use the external functions/types exported by the C API. 

covered the question from @qard so don't think we need to update to the C++ Wrapper API section.

Added this to the Proposed Action section:

• The API provided by the Node.js runtime will be in C. In addition a C++ wrapper which only contains inline funcctions and uses the C APIs so that the changes in the C++ wrapper would not require recompilation.

@Quad @ianwjhalliday @saghul @trevnorris @jasnell can you take another look and give me an LGTM if appropriate to land as "DRAFT" knowing it will change was we work through the PoC.

[saghul]

LGTM as a draft. One thing that the document should contain IMHO is the ABI guarantees. As in, when is the ABI allowed to be broken / modified? I guess with a semver major, right?

[mhdawson]

Ideally we don't break ABI at all. Semver major would be absolute minimum but we'd want it to be rare enough that semver major is one of the weaker requirements.

[mhdawson]

@trevnorris any chance I can get a second L.G.T.M from you to land as a draft ?

[mhdawson]

@nodejs/api can I get review/approval from anybody else in the group ?

[agnat]

Before diving into technical details some thoughts about the document. It has been edited many times without reworking the text in full. Here are some suggestions to make your work more accessible. borrows a TC members hat

The thing I'm missing most is information about the design decision process. I would like to better understand the "Why?"s and "How?"s behind the design. For example, I've seen a brief discussion of alternatives but found no comparison of their technical merits which would explain why this approach was taken. I'd like to know what options have been explored. This proposal contains important design decisions that will affect all node addons. I would like the proposal to mention not only the advantages but also the disadvantages of the approach taken.

I also recommend to rework the "Proposed Action" and "Background" sections. "Proposed Action" lists goals and "Background" contains a roadmap aka. "Proposed Actions" . Maybe be like:

Goals:

Describe what we would like to achieve in an abstract way, expanding each bullet point with a sentence or two. For example it is not clear to me what "VM agnostic" entails. First it sounds like different versions of v8, later (and digging deeper) it seems to include different JavaScript engines like SpiderMonkey &c. I definitely see evidence of feature creep and the actual scope is not entirely clear to me.

I also feel that "ABI stability" is (way) to vague. It's such a complex subject. I'd comment on what sort of independence we want to achieve. I'd also add a sentence about what ABI related issues will remain. Let's make it more thorough. The oracle document helps to understand the issue, but this proposal should draw the line.

In the introduction there is a list of undesirable features of the status quo. Which of those are we going to address?

Oh, and list the non-goals too. Any future goals that this proposal takes into account?

Proposed Action

Describe how we achieve the goals above. Reassemble and expand the roadmap. Maybe add a link to the nodejs/abi-stable-node#2 (and other?) milestone ticket.

Background:

Try to get rid of this section. Lift the important pieces, those that drive this proposal from the documents linked and include them here consolidating your work. In particular the google doc contains a lot of information that I feel belongs into the proposal. AFAICT, the rest is mostly "Goals" or "Proposed Action" stuff.

Sorry to propose all these text changes, but I think it is an ambitious proposal and it deserves more clarity.

[agnat]

The proposal describes the design by presenting two interfaces: 1. The C API and 2. the user (or addon developers) interface. I think a better view is to describe it in terms of subsystems or layers.

1. The abstraction layer between v8/node and the C API
2. A header only user interface layer with the C API on one side and a v8/node/nan style API on the user end.

It is these subsystems we should talk about, not the interfaces. Note that in this view the actual shape of the C API is largely irrelevant. It is internal. I think there is a comment somewhere along the lines: "Let's just let the C API evovle as needed. Later optimize it to be very generic and universal, so we don't have to break it again." I'm paraphrasing, but I believe this is the right approach. So, let's forget the actual interface for a second.

Also, forget the details of the other interface, the C++ wrapper API. We already know what it should look like. In a first step it should be as close as possible to what we currently have: v8 style type system, node and nan utilities, similar syntax, similar features. Just state your goals. No point in presenting an incomplete interface.

What really matters is the code in these new layers. That is our investment. We commit to develop and maintain these new subsystems. An honest attempt should be made to outline the amount and the complexity of the code, because that is one thing the TC needs to know.

Instead of presenting the interfaces I suggest linking to (PRs on) the current PoC implementations, so we can take a look:

• nodejs/abi-stable-node
• nodejs/abi-stable-node-addon-examples
• can not find the level down PoC branch

[mhdawson]

@agnat the kinds of suggestions you are making is exactly why I'd like to get this landed as a DRAFT. You could then submit PR's to make the enhancements you are suggesting and they could be discussed in the PR.

[jasnell]

This LGTM, we can update with follow on PRs

[mhdawson]

@agnat I'm going to go ahead and land the current version and then look to address your comments through PRs. Do you want to submit PRs for the suggested changes or would you like me to do that based on your comments ?

[agnat]

@mhdawson that is not how it is supposed to work. People spent time and effort working through this asking very valid questions in the comments above. Not only me.

Answering these questions in the comment section is not enough. Postponing them till later is not ok. These questions arise while reading the proposal. The proposal should answer them. Please respect the work reviewers put into this. Include their feedback and improve the document.

I've been following this PR since pinged on day one. I postponed the review time and again because the document is no help. Diving in anyway 17some days ago it took me more than 16 hours of work to develop a mental picture of what really is about to happen. It is the documents job to describe the system. Instead I had to work it out myself.

I'm ready to help with the document down the road, but as it stands it simply lacks the structure to do that.

1. The interface centric view will never be final until the thing is done. Describe the (path to a) system instead.
2. Stating that $X is incomplete in the document guarantees major misunderstandings. Nobody knows if you skipped a feature because you decided not to support it or if it is an oversight. Describe the scope.
3. Mention performance and maintenance implications.
4. Answer the questions above in the document.

Please provide a proper technical proposal we can work on. One we can actually discuss because it has some meat.

[mhdawson]

@agnat, I am going to need more insight into what you are suggesting before jumping into the work you are suggesting.

I think a higher bandwidth discussion between us would help me understand where/what you'd like changed. I'm thinking a 30 minute discussion were we go through and come up with the main headings in the doc together would go a long way in helping me understand the concrete changes that will address you concerns. Would that be possible ?

We can do that either one on one or during the weekly get together held by the group working on the PoC (Fridays at 2EST).

[brodybits]

Why define the C++ interface in the same place? I would rather see the C++ wrapper defined and/or implemented somewhere else. Simpler parts working together (UNIX philosophy). As an idea: could we make a new NAN version that provides the C++ wrapper?

[brodybits]

I would also like to see a couple more API functions/enhancements:

1. extern "C" equivalent to int node::Start(int argc, char *argv[])
2. API function to start node instance in a non-default uv_loop (with a new V8 Isolate)

1 is needed by C embedders while 2 could help enable true multithreading on Node.js.

I experimented with I would be happy to submit a new issue and maybe a PR on https://github.com/nodejs/abi-stable-node to explain what I mean by 2.

+1 for getting something landed, maybe something simple so people can start submitting PRs. What is the status? Any conclusions between @mhdawson / @agnat?

P.S. I would be happy to raise both 1 and 2 as new issue(s). :-1: for not landing a DRAFT.

[Fishrock123]

Tagging with ctc-agenda as discussed in the Node Interactive NA VM Summit meeting.

[jasnell]

@nodejs/ctc : This EP was discussed extensively during the VM Summit today at the Node Interactive Collaborator Summit. While this is still very much a work in progress, there has been sufficient progress towards a working prototype and a well defined direction. We should make a decision about whether this EP should be accepted as a draft (and this PR landed). The recommendation of the VM Summit participants is that this should be accepted as a draft.

[Trott]

@nodejs/ctc Note that the CTC is being asked to review this EP, so please review it carefully if you are able.

@jasnell @Fishrock123 Any reason not to post an issue in the CTC repo and call for a vote right now? Or is there a reason the process needs to wait for the actual meeting?

[jasnell]

@trott I'd like to make sure that there's at least an opportunity for the CTC members to ask any clarification questions but opening a vote is a good idea.

[mhdawson]

@bnoordhuis has a number of good suggestions. I also know that the API will still change as the PoC continues so I opened issues in the abi-stable-node repo to address those.

Earlier, before we had progressed the PoC to working code it was useful to have the functions in the eps to give people a "feel" for what it might look like. At this point I'm wondering if we should remove it and replace it with a pointer to the header in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/blob/api-prototype-6.2.0/src/node_jsvmapi.h as it will continue to evolve.

[jasnell]

+1 to landing this EPS as a draft and iterating on from there

[mhdawson]

Discussed in CTC meeting last week. Agreed to land unless objects posted here. Going to land now.

[mhdawson]

Squashed commits.