stream: autodetect direction #1964

vtjnash · 2018-08-28T18:18:13Z

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

addaleax · 2018-08-28T19:08:00Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1015/
Node.js CI: https://ci.nodejs.org/view/libuv/job/libuv-in-node/54/

refack · 2018-08-28T20:11:20Z

Could you devise a test for this?

santigimeno · 2018-08-29T09:10:34Z

Changes LGTM. A couple of questions / comments:

We'll need updating the documentation.
What's the semver-ness of this change? I mean, what if the caller of uv_tty_init() passes readable = 1 (now unused) on a O_WRONLY fd. The expectation until now would be having a readable stream. Should we do something about it?
When you say in the comment But we've already been asking the OS to tell us what they actually are, when we were doing that?

vtjnash · 2018-08-30T16:26:08Z

When you say in the comment But we've already been asking the OS to tell us what they actually are, when we were doing that?

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.

What's the semver-ness of this change? I mean, what if the caller of uv_tty_init() passes readable = 1 (now unused) on a O_WRONLY fd. The expectation until now would be having a readable stream. Should we do something about it?

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?

vtjnash · 2018-08-31T21:53:57Z

Added some tests for this and updated docs to remove reference to this (now unused) parameter.

cjihrig

Mostly LGTM. Should probably run the CI and Node-CI again, given the updates and the amount of red the last time around.

cjihrig · 2018-09-03T22:35:24Z

docs/src/guide/utilities.rst


-Set ``readable`` to true if you plan to use ``uv_read_start()`` on the stream.
+The ``unused`` parameter is now auto-detected.


This sentence doesn't really make sense without some context on what unused used to do.

cjihrig · 2018-09-03T22:35:39Z

docs/src/guide/utilities.rst

@@ -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)


I think this change calls for a .. versionchanged:: entry.

cjihrig · 2018-09-03T22:44:31Z

src/win/tty.c

@@ -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) {


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

vtjnash · 2018-09-05T17:22:26Z

Thanks @cjihrig, I made the requested updates.

vtjnash · 2018-09-12T21:03:43Z

bump

santigimeno

Other than the test failure, LGTM.

santigimeno · 2018-09-18T14:11:33Z

test/test-spawn.c

@@ -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));


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)

vtjnash · 2018-09-18T17:37:28Z

That's fascinating! I've updated the test to accept either behavior.

santigimeno · 2018-09-19T06:21:46Z

Ci: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1030/
https://ci.nodejs.org/view/libuv/job/libuv-in-node/59/

cjihrig · 2018-09-19T13:18:16Z

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>

santigimeno · 2018-09-19T16:21:45Z

Landed in: 956bf6b and 4049879. Thanks!

vtjnash · 2018-09-19T16:38:07Z

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>

addaleax approved these changes Aug 28, 2018

View reviewed changes

cjihrig approved these changes Sep 3, 2018

View reviewed changes

vtjnash added 3 commits September 5, 2018 13:15

win,tty: remove deadcode

d1af5a3

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.

stream: autodetect direction

ef84cbb

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

update docs for cjihrig comments

52f0ff1

vtjnash force-pushed the jn/detect-readable-flag branch from 9b8d8c4 to 52f0ff1 Compare September 5, 2018 17:21

santigimeno approved these changes Sep 18, 2018

View reviewed changes

cjihrig force-pushed the v1.x branch from c92be35 to abe9e01 Compare September 18, 2018 15:51

fixup! allow pipe syscall to be bidirectional

030c769

santigimeno closed this Sep 19, 2018

vtjnash deleted the jn/detect-readable-flag branch September 19, 2018 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: autodetect direction #1964

stream: autodetect direction #1964

vtjnash commented Aug 28, 2018

addaleax commented Aug 28, 2018

refack commented Aug 28, 2018

santigimeno commented Aug 29, 2018

vtjnash commented Aug 30, 2018

vtjnash commented Aug 31, 2018

cjihrig left a comment

cjihrig Sep 3, 2018

cjihrig Sep 3, 2018

cjihrig Sep 3, 2018

vtjnash commented Sep 5, 2018

vtjnash commented Sep 12, 2018

santigimeno left a comment

santigimeno Sep 18, 2018

vtjnash commented Sep 18, 2018

santigimeno commented Sep 19, 2018

cjihrig commented Sep 19, 2018

santigimeno commented Sep 19, 2018

vtjnash commented Sep 19, 2018


		Set ``readable`` to true if you plan to use ``uv_read_start()`` on the stream.
		The ``unused`` parameter is now auto-detected.

		@@ -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) {

stream: autodetect direction #1964

stream: autodetect direction #1964

Conversation

vtjnash commented Aug 28, 2018

addaleax commented Aug 28, 2018

refack commented Aug 28, 2018

santigimeno commented Aug 29, 2018

vtjnash commented Aug 30, 2018

vtjnash commented Aug 31, 2018

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Sep 3, 2018

Choose a reason for hiding this comment

cjihrig Sep 3, 2018

Choose a reason for hiding this comment

cjihrig Sep 3, 2018

Choose a reason for hiding this comment

vtjnash commented Sep 5, 2018

vtjnash commented Sep 12, 2018

santigimeno left a comment

Choose a reason for hiding this comment

santigimeno Sep 18, 2018

Choose a reason for hiding this comment

vtjnash commented Sep 18, 2018

santigimeno commented Sep 19, 2018

cjihrig commented Sep 19, 2018

santigimeno commented Sep 19, 2018

vtjnash commented Sep 19, 2018