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.

Sign up for GitHub

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

Jump to bottom

_get_osfhandle in Addon fails on Windows #6369

Closed
davedoesdev opened this issue Apr 25, 2016 · 9 comments
Closed

_get_osfhandle in Addon fails on Windows #6369

davedoesdev opened this issue Apr 25, 2016 · 9 comments
Labels
windows Issues and PRs related to the Windows platform.

Comments

@davedoesdev
Copy link
Contributor

  • Version: v5.10.1
  • Platform: Windows 7, 8 and 10 (32 and 64 bit)
  • Subsystem: fs and Addons

_get_osfhandle fails to map a file descriptor opened in Javascript to a handle.

Please see https://github.com/davedoesdev/node-windows-fd-test for a simple repro. It's also seen in baudehlo/node-fs-ext#57 and nodejs/node-gyp#416

I think this is due to MSVCR being statically linked into Node. _get_osfhandle can't find a descriptor opened in Javascript because the file descriptor to handle map in the Addon is separate to the one statically linked into the Node executable.

It would be nice to be able to use file descriptors passed into Addons on Windows. One idea is to expose _get_osfhandle (or a wrapper) from the Node executable for Addons to link against and call.
@ChALkeR ChALkeR added the windows Issues and PRs related to the Windows platform. label Apr 25, 2016
@bnoordhuis
Copy link
Member

Related: #6274. We need a way to re-export symbols that aren't marked __declspec(dllexport) but it's currently a right pain to do so.

@davedoesdev davedoesdev mentioned this issue Apr 27, 2016
Closed
@piscisaureus
Copy link
Contributor

The best way to go about it is probably to make the uv__get_osfhandle() function public.
https://github.com/libuv/libuv/blob/b12624c13693c4d29ca84b3556eadc9e9c0936a4/src/win/handle-inl.h#L165-L177

@bnoordhuis
Copy link
Member

I'd rather make node properly re-export symbols. We need that anyway to make linking against openssl possible on Windows. I've been nibbling at it but it's slow going.

@dimichgh
Copy link

dimichgh commented May 9, 2016 •
edited
Loading

I tried to port mmap.js to windows (https://github.com/dimichgh/mmap.js/blob/master/src/winmmap.h#L58), but due to this issue, I am now blocked.

This was referenced May 9, 2016
Open
Closed
@davedoesdev davedoesdev mentioned this issue Oct 12, 2016
Closed
@zbjornson zbjornson mentioned this issue Nov 7, 2016
Closed
@jviotti jviotti mentioned this issue Apr 5, 2017
Closed
@jviotti
Copy link
Member

jviotti commented Apr 5, 2017

This is hard blocker for https://github.com/resin-io/etcher. I opened an issue in libuv. I'm happy to contribute a patch to make uv__get_osfhandle() public if they agree: libuv/libuv#1291

@baudehlo
Copy link

It's a year to the day. Any progress on this?

@jviotti jviotti mentioned this issue Apr 26, 2017
Closed
@jviotti
Copy link
Member

jviotti commented Apr 26, 2017

@baudehlo I just sent a PR for this: libuv/libuv#1323

@addaleax addaleax mentioned this issue May 27, 2017
Closed
cjihrig pushed a commit to libuv/libuv that referenced this issue May 30, 2017
@cjihrig
win: make uv__get_osfhandle() public
e133923 
The uv__get_osfhandle() function is a private functio of the
Windows subsystem, and its used to get a Windows HANDLE out
of a file descriptor number.

The motivation behind making this function public is to
allow Node.js programs to pass file descriptors created
using fs.open() to native Node.js C++ add-ons, and be able to
successfully convert them to Windows HANDLEs.

Refs: #1166
Refs: nodejs/node#6369
Fixes: #1291
PR-URL: #1323
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bzoz
Copy link
Contributor

bzoz commented Jun 20, 2017

I guess this was solved by libuv/libuv#1323. Closing.

@bzoz bzoz closed this as completed Jun 20, 2017
jykahama added a commit to jykahama/mmap-io that referenced this issue Jun 26, 2017
@jykahama
Windows functionality & build corrections
a14d05e 
Functionality
* Use always page file backed MMF on windows (due to issue nodejs/node#6369).
* On windows pass page file name to mmap_map.
* Do not CloseHandle of mapped file until cleaning up as closing handle closes also connection to page file backed MMF.
* Throw unsupported error for mincore on windows

Windows build
* Compile flags around mincore calls
* npm prepublish script changed from 'make ls' to direct call of 'node lsc...'
@davedoesdev
Copy link
Contributor Author

@bzoz yes indeed, it does work. I've updated my test app (https://github.com/davedoesdev/node-windows-fd-test, see first comment in this ticket) to use uv_get_osfhandle.

@jviotti thanks for fixing this.

@davedoesdev davedoesdev mentioned this issue Jul 6, 2017
Closed
@bkmartinjr bkmartinjr mentioned this issue Dec 22, 2017
Merged
@primeare primeare mentioned this issue Apr 30, 2018
Closed
@CAMOBAP CAMOBAP mentioned this issue Jun 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@piscisaureus @bnoordhuis @ChALkeR @baudehlo @dimichgh @jviotti @davedoesdev @bzoz