Skip to content
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

No access to uv_get_osfhandle from N-API #318

Closed
primeare opened this issue Apr 30, 2018 · 8 comments
Closed

No access to uv_get_osfhandle from N-API #318

primeare opened this issue Apr 30, 2018 · 8 comments

Comments

@primeare
Copy link

The problem is that file descriptors returned by Node.js cannot be used to convert them into Windows file handlers (for instance using _get_osfhandle from Windows API). That problem was disscussed before in issues: node#6369 and libuv#1323. Libuv team has made uv__get_osfhandle function public to use inside of native addons, and I think this function solves the problem, but there is no port of this function into N-API.

@bnoordhuis
Copy link
Member

It would be better to obviate the need for uv_get_osfhandle() because it's really just a hack to get around the fact that add-ons link in their own copy of the CRT (the /MT link flag.)

Fix that by linking against node's copy and things like _get_osfhandle() and _open_osfhandle() magically start working.

@primeare
Copy link
Author

Thanks, @bnoordhuis! I was wondering how to make a fix that you said. Can you help me with this? What I need to add to my code or what I need to configure? I saw, that in libuv function uv_get_osfhandle() they simply turns off CRT assertion.

@bnoordhuis
Copy link
Member

Add-ons use node's common.gypi during the build; node-gyp downloads it. Because node builds with /MT, add-ons do too.

I haven't fully thought out how yet but add-ons should stop using /MT (could probably suppress it in node-gyp's addon.gypi) and start linking against the CRT from node.exe.

I don't know if node.exe currently re-exports symbols from msvcrt.lib. If the answer is "no, it doesn't", then that's the first order of business.

@primeare
Copy link
Author

Hmm @bnoordhuis, I discovered something interesting: I have tried to use uv_get_osfhandle function in my code and it works! I don't know how, but I think that's because somewhere in dependencies there is an include with that function. Anyway, what can we do to disable /MT flag in addons and start linking against the CRT from node?

@bnoordhuis
Copy link
Member

what can we do to disable /MT flag in addons and start linking against the CRT from node?

Experiment! :-) Clone node and node-gyp and see if you can make it work locally. Once you have an idea of what's involved, we can discuss next steps.

node-gyp accepts a --nodedir <path> argument. Point it to your node checkout to make it pick up your changes to common.gypi.

cjihrig added a commit to cjihrig/node that referenced this issue Aug 20, 2018
Notable changes:
- Restores compatibility with the old IPC protocol.
- Adds uv_open_osfhandle().
- Adds uv_os_{get,set}priority().

PR-URL: nodejs#22365
Fixes: nodejs#21671
Fixes: nodejs#15433
Refs: nodejs#21675
Refs: nodejs/node-addon-api#304
Refs: nodejs/abi-stable-node#318
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit to nodejs/node that referenced this issue Aug 21, 2018
Notable changes:
- Restores compatibility with the old IPC protocol.
- Adds uv_open_osfhandle().
- Adds uv_os_{get,set}priority().

PR-URL: #22365
Fixes: #21671
Fixes: #15433
Refs: #21675
Refs: nodejs/node-addon-api#304
Refs: nodejs/abi-stable-node#318
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit to nodejs/node that referenced this issue Sep 3, 2018
Notable changes:
- Restores compatibility with the old IPC protocol.
- Adds uv_open_osfhandle().
- Adds uv_os_{get,set}priority().

PR-URL: #22365
Fixes: #21671
Fixes: #15433
Refs: #21675
Refs: nodejs/node-addon-api#304
Refs: nodejs/abi-stable-node#318
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 5, 2018
Notable changes:
- Restores compatibility with the old IPC protocol.
- Adds uv_open_osfhandle().
- Adds uv_os_{get,set}priority().

PR-URL: nodejs#22365
Fixes: nodejs#21671
Fixes: nodejs#15433
Refs: nodejs#21675
Refs: nodejs/node-addon-api#304
Refs: nodejs/abi-stable-node#318
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit to nodejs/node that referenced this issue Nov 11, 2018
Notable changes:
- Restores compatibility with the old IPC protocol.
- Adds uv_open_osfhandle().
- Adds uv_os_{get,set}priority().

Backport-PR-URL: #24103
PR-URL: #22365
Fixes: #21671
Fixes: #15433
Refs: #21675
Refs: nodejs/node-addon-api#304
Refs: nodejs/abi-stable-node#318
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@simonbuchan
Copy link

As mentioned by @primeare it works fine to #include <uv.h>, but AFAIK that's not really considered part of N-API, and thus doesn't have ABI guarantees. But the existence of napi_get_uv_event_loop() is then a bit confusing - I'm guessing it's there as an escape hatch. Does that all sound right?

Personally I'm not sure if I'd like to see N-API be a general ECMAScript engine native API with node-specific escape hatches or to be quite node-concept specific. For example, should it expose crypto in some future version?

In any case, I think exposing napi_{get,open}_osfhandle() is probably better than trying to mess with link flags, as the latter adds a subtle dependency on the library ABI as part of the ABI guarantee N-API gives. If I understand correctly, for example, this means a debug build of node couldn't load addons without a rebuild, since it (I assume) uses /MTd, and using /MD means you're adding an external runtime dependency, where there wasn't one before (with VC++ moving to a stable libc/++ ABI, that's way less of an issue, but hey, still not great)

@mhdawson
Copy link
Member

napi_get_uv_event_loop is odd, we might have made a mistake on that one but I can't remember the history on that one.

@mhdawson
Copy link
Member

I'm going to close this as there has been no follow up from the op for quite some time. Closing, please let us know if that is not the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants