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

can the fs module operate on fd's opened by a native add-on? #15433

Closed
jorangreef opened this issue Sep 15, 2017 · 11 comments
Closed

can the fs module operate on fd's opened by a native add-on? #15433

jorangreef opened this issue Sep 15, 2017 · 11 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers.

Comments

@jorangreef
Copy link
Contributor

Is it safe to use a native add-on to open a file, and then pass the file descriptor to methods of the fs module?

My concern is especially for Windows. Will everything just work because Libuv uses uv__get_osfhandle?

Usually, people want to open a file in Node, and pass the file descriptor to a native add-on and this used to give problems on Windows before uv__get_osfhandle was made public.

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers. labels Sep 15, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Sep 15, 2017

I think if you use libuv to obtain the fd, and wrap it appropriately for use in JavaScript like the fs module does, you should be fine. If the way fs works ever changes, you could get broken though.

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

FWIW, I've done this and yes, it works and yes, it turns out to be a bad idea. Your best bet to to use fs to open/close the fd. At least for now.

@bnoordhuis
Copy link
Member

As well, it won't work if your add-on - or, more commonly, one of your add-on's libraries - is linked against a different CRT.

node.exe is linked with /MT, the static multi-threaded CRT. If a library is linked with e.g. /MD (dynamic multi-threaded), it gets a separate file descriptor table that won't be visible to uv__get_osfhandle().

@jorangreef
Copy link
Contributor Author

Thanks to everyone for the feedback.

node.exe is linked with /MT

Will node-gyp rebuild on Windows link with /MT by default?

I'm assuming that fds on Mac and Linux are no problem at all and that the "bad idea" part is really only with Windows?

The reason for asking is I'd like to add support for O_DIRECT and O_SYNC on Windows (via FILE_FLAG_NO_BUFFERING and FILE_FLAG_WRITE_THROUGH
respectively). See https://ayende.com/blog/174785/fast-transaction-log-windows for more details.

If converting Windows handles to fds and back is going to be problematic for a native add-on, then I don't mind doing the work to add this to Node's fs.open() directly, if someone else can step up to write the test.

@bnoordhuis
Copy link
Member

Will node-gyp rebuild on Windows link with /MT by default?

Yes (although any libraries your add-on links against may not be, of course.)

@jorangreef
Copy link
Contributor Author

I tried to open a file from a native add on, just doing this:

HANDLE file = CreateFileW(
  pathw,
  FILE_GENERIC_READ | FILE_GENERIC_WRITE,
  FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
  NULL,
  OPEN_EXISTING,
  FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH,
  NULL
);
free(pathw);
if (file == INVALID_HANDLE_VALUE) {
  return Nan::ThrowError("CreateFileW failed");
}
int fd = _open_osfhandle((intptr_t) file, _O_RDWR | _O_EXCL);
if (fd < 0) {
  CloseHandle(file);
  return Nan::ThrowError("_open_osfhandle failed");
}
info.GetReturnValue().Set(Nan::New<v8::Number>(fd));

That's all the native add on does. It's just there to open the file with Windows' O_DIRECT and O_DSYNC equivalents since these are not yet supported by libuv.

I get the fd returned successfully from the native add on, but when I pass it back to Node's fs.fstatSync() or any other fs method I get EBADF: bad file descriptor, fstat.

I thought the native add on would share the static multi-threaded CRT?

@bnoordhuis
Copy link
Member

Now that you mention it: add-ons probably inherit the /MT switch from node's common.gypi, which would mean they each get their own CRT copy. That would be... sub-optimal.

@nodejs/platform-windows Can one of you check that hypothesis?

@joaocgreis
Copy link
Member

add-ons probably inherit the /MT switch from node's common.gypi, which would mean they each get their own CRT copy.

Hypothesis checks out. Removing /MT from common.gypi makes the compiled modules much smaller, and they still seem to work well. However, they will have to use the CRT installed in the machine, and this is probably an issue when distributing compiled modules.

@bnoordhuis
Copy link
Member

@joaocgreis Anything we can do about that? I think ideally we'd want add-ons to use the CRT that's linked with node; i.e., we re-export the CRT's symbols. Are there downsides to that?

@eriohl
Copy link

eriohl commented Mar 2, 2018

I guess this one is pretty dormant but does anyone know a workaround for this problem? I have the same problem (although in my case I want to pass a pipe handle to uv_pipe_open).

(The only way I see this working is if node.exe exports _open_osfhandle as it does with _get_osfhandle (via uv_get_osfhandle) to convert the other way.)

bzoz added a commit to JaneaSystems/libuv that referenced this issue Jul 19, 2018
Adds uv_open_osfhandle to complete uv_get_osfhandle

Ref: nodejs/node#15433
Ref: nodejs/node-addon-api#304
@danielgindi
Copy link

@eriohl with named pipes specifically you have to be very careful, as they have multiple combinations of mode that could make trouble (PIPE_NOWAIT or TYPE_MESSAGE/PIPE_READMODE_MESSAGE)

Anyway there's a PR that was already approved for libuv, hopefully it will make it to nodejs soon!

bzoz added a commit to JaneaSystems/libuv that referenced this issue Aug 1, 2018
Adds uv_open_osfhandle to complete uv_get_osfhandle

Ref: nodejs/node#15433
Ref: nodejs/node-addon-api#304
bzoz added a commit to libuv/libuv that referenced this issue Aug 9, 2018
Adds uv_open_osfhandle to complete uv_get_osfhandle

Ref: nodejs/node#15433
Ref: nodejs/node-addon-api#304
PR-URL: #1927
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

8 participants