-
Notifications
You must be signed in to change notification settings - Fork 34
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
Problems with N-API that prevent us from using N-API exclusively, instead of V8 and Node internal APIs #279
Comments
To add to the wishlist: For https://github.com/mika-fischer/win32-service I need to call back into JS from a thread that's created by windows. Last I checked that was impossible to do with n-api. Thus I created a wrapper around uv_async_t for that: https://github.com/mika-fischer/napi-thread-safe-callback But it would be nice of course if n-api supported this use-case as well without resorting to libuv... |
Thanks for the input will add to the list here: #281 which we have in the current milestone. |
@digitalinfinity can you look at the windows use case ? |
@mhdawson I assume you refer to my comment above. Just to be clear, there's nothing Windows-specific about this. The issue is just: how do I call back into JavaScript from a thread other than the main thread (and also assuming I can't use the worker pool)? And currently this is impossible with n-api (although I have not yet checked what changed in 1.1). I mentioned the win32-service package just to illustrate that in cases like this, the requirement to call into JS from a different thread cannot be avoided. |
@mika-fischer is the some code in an existing module (obviously written without N-API) that does what you suggest that we can look at to see the pattern in more detail ? |
@mika-fischer or would that just be the module you referenced already. |
@mhdawson I wrote the module I cited and I wanted to use n-api, but for this functionality I had to resort to using libuv. So yes, this module is an example, even though it already uses n-api. I also wrote a helper class for this pattern, which can be found here: https://github.com/mika-fischer/napi-thread-safe-callback |
@mika-fischer thanks, will try to take a look over the next week or so. |
FYI possibly related and also has suggest API addition Possibly related, also with suggested API addition nodejs/node#13512. |
I think some of the issues have been addressed and it has been quite some time. I'm going to close this, please open a new issue either in the nodejs/node repo or nodejs/node-addon-api repos if the problems are still present for your use cases. Please let us know if you think that was not the right thing to do. |
First of all thanks for taking the direction of having a binary stable API. I'm really excited for the future of native add-ons.
I have just rewritten the zeromq.js bindings to use N-API (published at https://github.com/rolftimmermans/zeromq-ng).
There are two issues that I have found that are currently not possible to solve without relying on the internal APIs exposed by
v8.h
andnode.h
. I am opening this issue hear to give some greater visibility to the problems that I encountered.One of our use cases is to use
uv_poll_start
anduv_timer_start
to asynchronously wait on sockets to become readable. After that we need to resolve/reject a promise. This is currently not possible because the microtask queue does not run, see nodejs/node#15604. A workaround requires circumventing N-API by callingnode::CallbackScope
directly.Another use case is to call a callback function after
uv_poll_start
anduv_timer_start
. This also does not work correctly – it fails if the callback throws, see nodejs/node#15371. I have not found a suitable workaround! Although this code path has since been removed from thezeromq-ng
library, it might be very relevant for others.These two problems currently prevent the zeromq library from using N-API exclusively. It would be great if this could be somehow addressed before N-API stabilises... If someone can give pointers towards possible solutions I'd be happy to open a PR.
The text was updated successfully, but these errors were encountered: