LEP-0005 implementation – remove uv_file #1166

vtjnash · 2016-12-10T01:38:27Z

Since on Windows, HANDLEs are not the same as file descriptors,
confusing the two meant that almost all APIs in libuv had to go through
a thin compatibility layer, adding needless complication.

This removes that layer of external indirection and instead
works with the native Win32 API directly.

This is the implementation for libuv/leps#8 which closes #856.

APIs added:

uv_get_osfhandle
uv_convert_fd_to_handle
uv_dup
UV_STDIN_FD / UV_STDOUT_FD / UV_STDERR_FD

APIs removed:

uv_poll_init_socket (now identical to uv_poll_init)

TODO list:

documentation updates
merge LEP 5

bnoordhuis · 2016-12-12T12:46:15Z

It's not clear to me why you removed uv_poll_init(). It's used a lot, removing it is rather disruptive.

vtjnash · 2016-12-12T13:25:24Z

On Windows, it's an alias for uv_poll_init_socket (on non-windows, the aliasing is reversed). In this case, I opted for removing the name uv_poll_init where the API changed on Windows (and keeping the name uv_poll_init_socket, which isn't affected by this change), to help FFI users find places they need to fix their code. But I can make this go either way, and I opened this as WIP to get feedback on that.

saghul · 2016-12-12T13:58:53Z

I'd rather keep uv_poll_init for consistency with the rest of the handles.

saghul

Made a first review round! I'm happy with the code reduction and how fs operations are simpler :-)

I left a bunch of comments but nothing major. Please address them, rebase and I'll kick off a CI run and give it another review round.

saghul · 2017-02-19T20:35:50Z

include/uv.h

@@ -431,10 +427,56 @@ UV_EXTERN int uv_send_buffer_size(uv_handle_t* handle, int* value);
 UV_EXTERN int uv_recv_buffer_size(uv_handle_t* handle, int* value);

 UV_EXTERN int uv_fileno(const uv_handle_t* handle, uv_os_fd_t* fd);
+UV_EXTERN int uv_dup(uv_os_fd_t fd, uv_os_fd_t* dup);


can you rename the parameter to dupfd

saghul · 2017-02-19T20:36:18Z

include/uv.h

+#endif
+
+INLINE static uv_os_fd_t uv_get_osfhandle(int fd)
+{


style: brace on the previous line

saghul · 2017-02-19T20:36:42Z

include/uv.h

+{
+#ifdef _WIN32
+  /* _get_osfhandle() raises an assert in debug builds if the FD is invalid. */
+  /* So if you plan on using invalid fd, you will need to install a _CrtSetReportHook handler */


make this a block comment

Should we have that handler added automaticaly?

No, libuv can't add that handler automatically as the _CrtSetReportHook may be non-unique in the process. Since libuv won't use file-descriptors after this change, it'll never see an invalid one.

saghul · 2017-02-19T20:37:08Z

include/uv.h

+
+
+INLINE static uv_os_fd_t uv_convert_fd_to_handle(int fd)
+{


style: braces

saghul · 2017-02-19T20:38:57Z

include/uv.h

+# define INLINE inline
+#endif
+
+INLINE static uv_os_fd_t uv_get_osfhandle(int fd)


Please define these in uv-common.c, no need to inline them, IMHO

Inlining them is the only way to get the linker to point to the right copy of this function.

saghul · 2017-02-19T21:05:34Z

src/win/fs.c

        result = -1;
        break;
      }
+      /* hopefully n == n_out here (e.g. that fd_out wasn't opened non-blocking mode) */


shouldn't we account for the case where that doesn't happen? if the answer it's "never", place an assert

This is just documenting an existing potential deficiency (_write is just a thin wrapper for WriteFile, although I shudder to think of what this code used to do if someone accidentally opened a fd with _O_TEXT ). I don't think anyone is passing in a pipe opened in non-blocking mode intentionally.

saghul · 2017-02-19T21:06:23Z

src/win/fs.c

  } else {
    fs__open(req);
-    return req->result;
+    if (req->result < 0)


i think the previous version was more readable, please revert this

The old version does a silent truncating cast (from HANDLE to int), #1166 (comment)

saghul · 2017-02-19T21:08:47Z

src/win/process-stdio.c

-          if (fdopt.data.fd <= 2 && err == ERROR_INVALID_HANDLE) {
+          /* If fdopt.data.file is pointing at one of the pseudo stdio handles,
+           * but it is not valid ignore the error. */
+          if ((fdopt.data.file == UV_STDIN_FD || fdopt.data.file == UV_STDOUT_FD || fdopt.data.file == UV_STDERR_FD) &&


style: long line

saghul · 2017-02-19T21:09:34Z

src/win/tty.c

     */
+    handle = GetStdHandle((DWORD)(uintptr_t) handle);


this construct seems to repeat a bit, maybe create an internal uv__dup_stdio_handle helper?

saghul · 2017-02-19T21:17:45Z

test/test-ipc-send-recv.c

@@ -376,7 +376,8 @@ int run_ipc_send_recv_helper(uv_loop_t* loop, int inprocess) {
    r = uv_listen((uv_stream_t*)&ctx2.listen, SOMAXCONN, listen_cb);
    ASSERT(r == 0);
  } else {
-    r = uv_pipe_open(&ctx2.channel, 0);
+    uv_os_fd_t stdin_handle = uv_convert_fd_to_handle(0);


doesn't UV_STDIN_FD work here?

It would work, since for the purposes of this test harness they happen to get set to the same stream. However, testing with fd = 0 is slightly more accurate – and makes for better example code for the expected nodejs usage.

vtjnash · 2017-02-20T21:23:30Z

I've updated the PR based on your review feedback. Let me know what else needs to be done here.

saghul · 2017-03-23T08:10:50Z

Hey @vtjnash I'm ready to land this, once we have a bit of documentation. I seem to have forgotten to point that out earlier, sorry about that.

We need:

docs on the new utility function
corrections an all other functions that changed
an entry in a v1 -> v2 migration guide (this would start it) similar to this: http://docs.libuv.org/en/v1.x/migration_010_100.html

vtjnash · 2017-03-24T10:15:10Z

Thanks @sagful. I had documentation update in my TODO list at the top, and was just waiting for the LEP to be merged. I also added the migration guide – thanks for mentioned that too. Let me know if it looks good (or has any typos :P).

saghul

Some more documentation fixes are required. Also please drop the public uv_dup.

saghul · 2017-04-03T08:35:32Z

docs/src/handle.rst

@@ -179,6 +179,12 @@ just for some handle types.
        Be very careful when using this function. libuv assumes it's in control of the file
        descriptor so any change to it may lead to malfunction.

+.. c:function:: int uv_dup(uv_os_fd_t fd, uv_os_fd_t* dupfd)


I see we only use this internally, please make it private.

saghul · 2017-04-03T08:36:20Z

docs/src/fs.rst

+
+   Destructively converts a file descriptor in the C runtime
+   to the OS-dependent handle.
+   Upon return, the input ``fd`` parameter is invalid and closed.


please add some more details mentioning this only makes sense on windows, what function uses internally, etc.

saghul · 2017-04-03T08:36:43Z

docs/src/fs.rst

@@ -161,7 +161,7 @@ API
    Cleanup request. Must be called after a request is finished to deallocate
    any memory libuv might have allocated.

-.. c:function:: int uv_fs_close(uv_loop_t* loop, uv_fs_t* req, uv_file file, uv_fs_cb cb)
+.. c:function:: int uv_fs_close(uv_loop_t* loop, uv_fs_t* req, uv_os_fd_t file, uv_fs_cb cb)


add a .. versionchanged:: for each of these

Should that be v2.0.0, or something else?

I've added a fixup! commit to address the doc comments, assuming for now that I should go with v2.0.0.

Yep, that's the one, thanks!

saghul · 2017-04-03T08:37:41Z

docs/src/migration_100_200.rst

+Constants ``UV_STDIN_FD``, ``UV_STDOUT_FD``, and ``UV_STDERR_FD`` provide cross-platform
+references to the standard constants for stdio access, and can be used in place of any ``uv_os_fd_t``.
+Existing clients can transition to the new API by using the conversion function ``uv_convert_fd_to_handle``
+defined in the libuv header file.


please expand a bit here: show an example on how one would open stdin before and how to do it now.

saghul · 2017-04-03T08:40:00Z

include/uv.h

+# define INLINE inline
+#endif
+
+INLINE static uv_os_fd_t uv_get_osfhandle(int fd) {


please move these to uv-common. We tend not to have implementations in uv.h.

These need to be statically linked into the caller, which is why they can't go in the libuv library.

The recently opened issue #1291 demonstrates why it is required that this code be part of the header and not the library.

I see! Can you please add a comment so someone doesn't f*ck this up in the future?

Yes, added.

saghul · 2017-04-24T07:07:56Z

Damn, GH doesn't send notifications when you add new commits so I missed this. I'll review today @vtjnash.

saghul

LGTM once uv_dup is removed from the public API. That's the only thing left, fantastic work here!

vtjnash · 2017-04-26T16:51:01Z

Ok, I added a commit to remove uv_dup from the public API (I'm assuming you'll squash when you merge).

vtjnash · 2017-05-15T20:06:05Z

bump.

vtjnash · 2017-05-24T02:14:37Z

All comments have been addressed, should be ready to merge.

saghul · 2017-05-29T18:40:34Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/278/

vtjnash · 2017-05-29T19:13:36Z

Thanks - looks like I need to add the io.h header.

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>

Since on Windows, HANDLEs are not the same as file descriptors, confusing the two meant that almost all APIs in libuv had to go through a thin compatibility layer, adding needless complication. This removes that layer of external indirection and instead works with the native Win32 API directly.

vtjnash · 2017-06-23T22:59:13Z

Alright, I've tried to cleanup the list of headers affected by this change to help avoid accidentally referencing shared state in msvcrt.dll where not intended. I think this should address the CI test failure, but would be best to re-run that to check that I didn't miss any.

santigimeno · 2017-06-26T12:58:48Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/299/

santigimeno · 2017-06-27T07:19:42Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/301/

santigimeno · 2017-06-28T07:43:33Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/305/

santigimeno · 2017-06-28T08:28:28Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/306/

saghul · 2017-07-04T20:47:27Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/314/

saghul · 2017-07-04T21:40:49Z

💚 landing!

Since on Windows HANDLEs are not the same as file descriptors, confusing the two meant that almost all APIs in libuv had to go through a thin compatibility layer, adding needless complication. This removes that layer of external indirection and instead works with the native Win32 API directly. See: https://github.com/libuv/leps/blob/master/005-windows-handles-not-fd.md PR-URL: #1166 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul · 2017-07-04T21:47:11Z

Landed in 68dafe9, cheers @vtjnash! This was a lot of work and it took a long time to review, I'm sorry about the delay on my side and I'm grateful you stuck with it and took it over the finish line. 🍸

saghul · 2017-07-04T21:48:20Z

@libuv/collaborators I'll try to merge v1.x into master this weekend. It will be a messy one after this commit, but once done the next ones should be easier.

vtjnash · 2017-07-04T23:16:36Z

😀😀😀💥💥💥Thanks!

It was already wrong inside libuv#974 (`UV_EINVAL` and `-EINVAL` are different on Windows), but libuv#1166 made it even worse. It's now wrong on windows and *nix 😆

Refs: #974 Refs: #1166 PR-URL: #1446 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul added v2 windows labels Dec 10, 2016

saghul requested changes Feb 19, 2017

View reviewed changes

saghul requested a review from piscisaureus February 19, 2017 21:20

vtjnash force-pushed the jn/kill-msvcrt branch from 39203ac to 94707fd Compare February 20, 2017 20:58

saghul mentioned this pull request Mar 21, 2017

Pass Windows handles as STDIO to child process. #1119

Closed

vtjnash force-pushed the jn/kill-msvcrt branch from 94707fd to c25dcb7 Compare March 24, 2017 10:12

vtjnash changed the title ~~WIP: LEP-0005 implementation – remove uv_file~~ LEP-0005 implementation – remove uv_file Mar 24, 2017

saghul requested changes Apr 3, 2017

View reviewed changes

moteus mentioned this pull request Apr 5, 2017

Make uv__get_osfhandle() public #1291

Closed

saghul approved these changes Apr 25, 2017

View reviewed changes

vtjnash force-pushed the jn/kill-msvcrt branch from d6b58af to 1b24dd7 Compare April 26, 2017 16:43

jviotti mentioned this pull request Apr 26, 2017

win: make uv__get_osfhandle public #1323

Closed

vtjnash added 4 commits June 23, 2017 14:41

fixup! LEP-0005: remove uv_file

32c2413

add comment about static-inline usage in uv.h

ba809e4

remove uv_dup from the public api

a135b9b

fixup! use uv.h declared type for buf_used instead of off_t

2b86ad9

fixup! cleanup type-casts in tests

c26b68b

fixup! more attempted corrections

f01acd9

fixup! use alternate inline keyword for gcc for ansi-compliance

72b2643

saghul closed this Jul 4, 2017

fdopen mentioned this pull request Jul 29, 2017

loop-watcher: return negative number on error #1446

Closed

cjihrig pushed a commit that referenced this pull request Jul 31, 2017

src: return UV_EINVAL in uv_##name##_start()

9834c4f

Refs: #974 Refs: #1166 PR-URL: #1446 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

This was referenced Aug 21, 2017

Call uv_fs_open faild #1420

Closed

win: replace uv_file API #856

Closed

libuv Windows : uv_fs_init changes default file reading behavior of the current process. #840

Closed

stream: limit unix read/write to 2GB at a time #1501

Open

kozlovic mentioned this pull request Sep 29, 2017

added uv_poll_init_scoket for windows nats-io/nats.c#98

Closed

vtjnash deleted the jn/kill-msvcrt branch November 15, 2017 15:27

santigimeno mentioned this pull request Dec 29, 2017

Merge v1.16.1 into master #1630

Closed

vtjnash mentioned this pull request Aug 27, 2018

Merge 1.23.0 #1952

Closed

vtjnash mentioned this pull request Aug 14, 2019

pipe: add uv_pipe and uv_socketpair to the API #1498

Closed

yesudeep mentioned this pull request Sep 16, 2021

[Windows + MSVC 14.29.30133] usockets.lib(libuv.obj) : error LNK2001: unresolved external symbol uv_poll_init_socket uNetworking/uSockets#159

Closed

LEP-0005 implementation – remove uv_file #1166

LEP-0005 implementation – remove uv_file #1166

Conversation

vtjnash commented Dec 10, 2016 • edited Loading

bnoordhuis commented Dec 12, 2016

vtjnash commented Dec 12, 2016

saghul commented Dec 12, 2016

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Feb 20, 2017

saghul commented Mar 23, 2017

vtjnash commented Mar 24, 2017

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Apr 24, 2017

saghul left a comment

Choose a reason for hiding this comment

vtjnash commented Apr 26, 2017

vtjnash commented May 15, 2017

vtjnash commented May 24, 2017

saghul commented May 29, 2017

vtjnash commented May 29, 2017

vtjnash commented Jun 23, 2017

santigimeno commented Jun 26, 2017

santigimeno commented Jun 27, 2017

santigimeno commented Jun 28, 2017

santigimeno commented Jun 28, 2017

saghul commented Jul 4, 2017

saghul commented Jul 4, 2017

saghul commented Jul 4, 2017

saghul commented Jul 4, 2017

vtjnash commented Jul 4, 2017

vtjnash commented Dec 10, 2016 •

edited

Loading