Redesign async helper APIs

[boingoing]

Add error handling for the async helper APIs. At least they all return napi_status now.

Remove the individual APIs for setting all the pieces of the napi_work struct opting to just take all the parameters in napi_create_async_work. It doesn't seem like any of the parameters are optional here anyway.

Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway.

Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking.

Also added some async unit tests for addons-napi based on the addons/async_hello_world test.

[jasongin]

@boingoing also pointed out offline that these APIs need to add support for napi_get_last_error_info().

In my opinion, we should consider removing these APIs from the node PR for now, instead of rushing to get all these changes ready in time. Then we can take the time to get the design right later, potentially as part of a broader treatment for libuv APIs. (If we still think the abstraction is needed at all for libuv at all.)

[mhdawson]

I see lots of good improvements in this PR/discussion and I think we should continue to refine and then see what it looks like. I don't think we should rush to remove, Anna reacted positively to the explanation for why they were there. I really think we should avoid the case were we lead module maintainers down the path of in-inadvertently pulling in methods which are not ABI safe. If exclude these I think we'll need the C++ wrapper to clearly state up front that it uses non-abi stable methods and as such does not preserve the ABI stability goal. I don't think that would be a good starting point for the wrapper or N-API.

[jasongin]

If exclude these I think we'll need the C++ wrapper to clearly state up front that it uses non-abi stable methods and as such does not preserve the ABI stability goal.

But libuv APIs are ABI-stable. Its last breaking change was pre-1.0. While hypothetically it could break compatibility with a 2.x release, no 2.x is planned as far as I know, and may never happen.

Also, if this specific issue is a concern, we could consider removing the AsyncWorker class from the C++ wrapper library. Its implementation is a fairly trivial composition of other N-API wrappers (Napi::ObjectReference and Napi::FunctionReference) with the libuv async work callbacks.

[addaleax]

While hypothetically it could break compatibility with a 2.x release, no 2.x is planned as far as I know, and may never happen.

A 2.x will happen at some, afaik yet indeterminate, point in the future. No changes to the APIs relevant for async work are currently in libuv/libuv@master and no open PRs with related changes exist, though.

[boingoing]

Pushed a commit addressing some review feedback and iterating on the design a little bit. Sorry in advance, this moves big blocks of code and deletes a couple of files so I realize reviewing the diff will be trickier.

Here's the commit message:

Remove node_api_async files
Put the async N-API methods into node_api.h/cc so they can access the error handling helpers without having to rearrange that code. I'm not committed to having all our methods in one set of files but this is a relatively small set of things to have an entire parallel set of files to support.

Add napi_cancel_async_work for cancelling a napi_async_work.

Call the complete handler for the async work item with a napi_status code translated from the uvlib error code.

Made the execute callback required for napi_create_async_work, though complete callback is still optional.

Also removed the destroy callback as there doesn't seem to be a reason to support it.

[kkoopa]

Also, if this specific issue is a concern, we could consider removing the AsyncWorker class from the C++ wrapper library. Its implementation is a fairly trivial composition of other N-API wrappers (Napi::ObjectReference and Napi::FunctionReference) with the libuv async work callbacks.

No, no. It is nowhere near a trivial composition for the average person developing a native addon. The primary reason for writing a native addon is necessity. Many addons are cobbled together from looking at existing addons, without much understanding of what goes on or what errors there are. These then trickle up the dependency chain and break the ecosystem. There is a definite need of cookie-cutter functionality for common use cases, which ensures that things are done right and reduce boilerplate.

Besides ABI stability, I do not consider it good that writing a native addon correctly requires intimate knowledge of the node.js API, the libuv API as well as how they interact. There should be one node.js API with one set of documentation and sensible names that fit together and make a unified whole. Writing an asynchronous native node.js addon should not even require knowing that libuv exists or what it is.

[jasongin]

@kkoopa The suggestion about removing AsyncWorker was only meant to be temporary, probably only a few days up to a week or two. Sorry if that wasn't clear. We know that the coverage of N-API is not yet complete anyway, which is why it going in as "experimental" status.

Another way to look at it is just breaking the one big PR into two smaller ones that would land separately, to reduce the scope and complexity of each piece.

[aruneshchandra]

We had a quick core team sync-up on this issue this morning - here's the takeaway.

There are known gaps in the experimental N-API version today. These gaps are both V8 and libuv related, and we know we have to address them as this project moves along and exits experimental status. We all agree that, at least in the short term, adding ABI stable layer over NAN supported libuv APIs is important for N-API promise and to provide NAN equivalency in its usage. However, decoupling async helper from the big original PR makes sense at this time for the following reasons:

• Allows us to carefully redesign and test async helper and not rush it in
• Allows us to make forward progress on the original PR without this being a blocker

We decided to temporarily take out async helper from the original PR and commit to prioritizing the redesign work and open a new PR with the right design and validation done. (Hopefully, immediately after the original PR lands!)

[kkoopa]

I see. I did not understand that it was merely temporary. To clarify my second statement, I was trying to argue for keeping the libuv wrappers for the sake of encapsulation and consistency, but agree that it is not essential for the first implementation, especially given the short release window. Provided that the experimental API makes it into node.js 8.0, what will its semver policy be from node core's side? Is it completely unaffected, partially unaffected, or wholly affected?

…

On March 29, 2017 7:39:18 PM GMT+03:00, Jason Ginchereau ***@***.***> wrote: @kkoopa The suggestion about removing AsyncWorker was only meant to be _temporary_, probably only a few days up to a week or two. Sorry if that wasn't clear. We know that the coverage of N-API is not yet complete anyway, which is why it going in as "experimental" status. Another way to look at it is just breaking the one big PR into two smaller ones that would land separately, to reduce the scope and complexity of each piece. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #204 (comment)

[jasongin]

Provided that the experimental API makes it into node.js 8.0, what will its semver policy be from node core's side?

Experimental features in node are explicitly not subject to semver policy, so they may change at any time. The header comment in node_api.h and the console output when enabling the feature via --napi-modules both warn about that.

In practice we will of course try to minimize breaking changes, as they are disruptive to anyone trying out the experimental feature. There are likely to be a few breaking changes early on, but stabilization is obviously a requirement for graduating out of experimental status. Much more common will be incremental addition of new APIs to expand coverage so that it is sufficient for a wider range of native addons. I don't think we've discussed yet whether we will try to align those kind of additions with semver-minor updates to node 8.x, even if we are technically not required to.

[jasongin]

we need to validate by bringing our ported modules up to date

Yes. And doing that will require an updated C++ wrapper that uses these new APIs. I'll prepare those changes in a branch.

[sampsongao]

It seems like we got some trailing white spaces.

[sampsongao]

node_api.cc:(.text+0x93e7): undefined reference to 'node::Environment::GetCurrent(v8::Isolate*)'
node_api.cc:(.text+0x93ef): undefined reference to 'node::Environment::event_loop() const'

Seems like we need to include env-inl.h

[jasongin]

Updates here: nodejs/node-addon-api#10, tested with the NAPI node-canvas port.

[jasongin]

@sampsongao I didn't see any build errors when I built with this PR on Windows. Maybe it is compiler-dependent? It does look like evn-inl.h should be required, if Environment::event_loop() is being inlined.

[sampsongao]

After suggested changes, nanomsg is running find with the new async api.

[mhdawson]

Landed in main repo closing.