-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
stream: autodetect direction #1964
Conversation
Could you devise a test for this? |
Changes LGTM. A couple of questions / comments:
|
The non-blocking state (O_NONBLOCK) is part of the same flags, so libuv is already dependent on this operation succeeding per POSIX.1-2001. I had been somewhat concerned that some system would not not define or report these flags correctly, but from the CI reports above, that does not appear to be the case.
That's an interesting question. That may be the expectation, but realistically, any actual attempt to read from it would have been blocked by the kernel. I think this could have the potential to change where and how that error would get reported (since we'll now know up-front that it'll fail). So I think this doesn't affect semver? |
Added some tests for this and updated docs to remove reference to this (now unused) parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. Should probably run the CI and Node-CI again, given the updates and the amount of red the last time around.
docs/src/guide/utilities.rst
Outdated
|
||
Set ``readable`` to true if you plan to use ``uv_read_start()`` on the stream. | ||
The ``unused`` parameter is now auto-detected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't really make sense without some context on what unused
used to do.
@@ -370,9 +370,9 @@ terminal information. | |||
The first thing to do is to initialize a ``uv_tty_t`` with the file descriptor | |||
it reads/writes from. This is achieved with:: | |||
|
|||
int uv_tty_init(uv_loop_t*, uv_tty_t*, uv_file fd, int readable) | |||
int uv_tty_init(uv_loop_t*, uv_tty_t*, uv_file fd, int unused) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change calls for a .. versionchanged::
entry.
@@ -382,12 +386,6 @@ int uv_tty_set_mode(uv_tty_t* tty, uv_tty_mode_t mode) { | |||
} | |||
|
|||
|
|||
int uv_is_tty(uv_file file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a separate commit.
Not used anywhere or exported. Most of this code also cares which direction handle is open too (Input or Output), so it's not particularly useful.
Previously, we required the user to specify the expected read/write flags for a pipe or tty. But we've already been asking the OS to tell us what they actually are (fcntl F_GETFL), so we can hopefully just use that information directly. Fix libuv#1936
9b8d8c4
to
52f0ff1
Compare
Thanks @cjihrig, I made the requested updates. |
bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the test failure, LGTM.
test/test-spawn.c
Outdated
@@ -1751,6 +1751,14 @@ TEST_IMPL(spawn_inherit_streams) { | |||
ASSERT(uv_pipe_open(&pipe_stdout_child, fds_stdout[1]) == 0); | |||
ASSERT(uv_pipe_open(&pipe_stdin_parent, fds_stdin[1]) == 0); | |||
ASSERT(uv_pipe_open(&pipe_stdout_parent, fds_stdout[0]) == 0); | |||
ASSERT(uv_is_readable((uv_stream_t*) &pipe_stdin_child)); | |||
ASSERT(!uv_is_writable((uv_stream_t*) &pipe_stdin_child)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is failing on FreeBSD and SmartOS as on these systems pipe()
returns a bidirectional data channel. See: https://www.freebsd.org/cgi/man.cgi?pipe(2) and https://illumos.org/man/2/pipe.
The failure is:
not ok 203 - spawn_inherit_streams
# exit code 134
# Output from process `spawn_inherit_streams`:
# Assertion failed in test/test-spawn.c on line 1755: !uv_is_writable((uv_stream_t*) &pipe_stdin_child)
That's fascinating! I've updated the test to accept either behavior. |
Hopefully one last Node CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/60/ (Just to double check on https://ci.nodejs.org/job/node-test-binary-windows/20101/COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=0/console) |
Not used anywhere or exported. Most of this code also cares which direction handle is open too (Input or Output), so it's not particularly useful. PR-URL: #1964 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Previously, we required the user to specify the expected read/write flags for a pipe or tty. But we've already been asking the OS to tell us what they actually are (fcntl F_GETFL), so we can hopefully just use that information directly. Fixes: #1936 PR-URL: #1964 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Yay, Thanks! :) |
Previously, we required the user to specify the expected read/write flags for a pipe or tty. But we've already been asking the OS to tell us what they actually are (fcntl F_GETFL), so we can hopefully just use that information directly. Fixes: libuv/libuv#1936 PR-URL: libuv/libuv#1964 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Previously, we required the user to specify the expected read/write flags for a pipe or tty. But we've already been asking the OS to tell us what they actually are (posix standard fcntl F_GETFL), so we can hopefully just use that information directly.
Fix #1936