windows: fix compatibility with cygwin pipes #1067

vtjnash · 2014-01-02T22:32:22Z

This makes libuv more tolerant of the properties of the pipes
that it can use. this is necessary because Cygwin (and hence Mintty)
opens STDIN without FILE_WRITE_ATTRIBUTES. Also, this always duplicate
handles from external sources, to ensure the original handle can remain
valid after uv_close (this can be important for stdio since the system
libraries do not expect it to be closed by the user).

Nodejs-Jenkins · 2014-01-02T22:32:24Z

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

Jameson Nash

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

…nnected. also, duplicate handles to ensure the original handle remains valid after uv_close (esp. important for stdio)

txdv · 2014-01-02T23:56:23Z

Everything after the : should go into a new line, because it is a small description of the headline.

vtjnash · 2014-01-03T00:02:57Z

@txdv are you looking at my second referenced commit? It's not clear from GitHub's view, but it was in a working branch (before I cleaned up the commit description to keep Jenkins from complaining).

saghul · 2014-02-07T08:42:04Z

/cc @piscisaureus

Keno · 2014-02-22T22:09:37Z

bump

oberlies · 2014-05-14T15:47:34Z

Would this be fixing nodejs/node-v0.x-archive#6459 ?

saghul · 2014-05-14T15:52:24Z

Looks like it would. Unfortunately this is way out of my Windows knowledge so we'd need some imput on it. @piscisaureus, @orangemocha does this look like a good approach?

piscisaureus · 2014-05-14T18:02:18Z

Hey @vtjnash, it's hard for me to review this code because I don't understand what the issues are with cygwin pipes and your code has no comments that explain it.

Of course I can now bitch about how you're duplicating handles without ever closing them and code style etc. but before that I'd like to understand it better.

Can you maybe explain the issue that this solves; if that's unpractical we can maybe make an appointment so I'll be on IRC (or some other medium) and we can chat about it.

vtjnash · 2014-05-15T03:50:17Z

src/win/pipe.c

@@ -230,7 +230,7 @@ static int uv_set_pipe_handle(uv_loop_t* loop,
  NTSTATUS nt_status;
  IO_STATUS_BLOCK io_status;
  FILE_MODE_INFORMATION mode_info;
-  DWORD mode = PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT;
+  DWORD mode = PIPE_READMODE_BYTE | PIPE_WAIT;


PIPE_TYPE_BYTE isn't a valid flag, although it didn't matter since it's value is 0.

vtjnash · 2014-05-15T03:56:20Z

A portion of this patch was independently executed in ba47e68

I've reworked (shortened) this to incorporate those changes, and join IRC when I have availability (usually late in the evenings).

It looks like I had some rebase issues when I copied this from the Julia fork (we have other changes there that fix the read/write flags of pipes on other platforms and the behavior of uv_spawn for all platforms per #451. I removed the changes -- such as duplicating handles from the host process -- that aren't directly relevant)

vtjnash · 2014-05-15T04:01:11Z

src/win/pipe.c

@@ -246,11 +246,9 @@ static int uv_set_pipe_handle(uv_loop_t* loop,
      if (!GetNamedPipeHandleState(pipeHandle, &current_mode, NULL, NULL,
                                   NULL, NULL, 0)) {
        return -1;
-      } else if (current_mode != mode) {
+      } else if (current_mode&PIPE_NOWAIT) {


previously, this verified that the pipe is in PIPE_READMODE_BYTE, but cygwin puts it in PIPE_READMODE_MESSAGE (IIRC, this was changed because it gives better behavior when there are multiple writers) and doesn't give us the permissions to edit the mode. however this should be fine, since we can always just read bytes at a time.

Style: spaces around '&'

This makes libuv more tolerant of the properties of the pipes that it can use without any issue. this is necessary because Cygwin (and hence Mintty) opens STDIN without FILE_WRITE_ATTRIBUTES.

Keno · 2014-06-03T17:12:05Z

BUMP

Keno · 2014-06-11T22:15:29Z

Bump^2 ;). If there's anything we can clarify, please let us know.

saghul · 2014-06-18T08:25:39Z

The new cleaned-up code LGTM. It just takes care of settig the readable/writable flags approriately. Can you do a quick lookover, @piscisaureus?

PS: There are a few style issues (mostly space between operators), we can fix those during merge.

piscisaureus · 2014-06-18T11:52:25Z

@Keno @vtjnash

Ok, I understand what you're trying to do now and it seems good.

I would really like it if you could add a comment to the code stating that cygwin opens the pipe in message mode but we can support that just fine. That way the knowledge in our heads doesn't get lost - you've probably already noticed that I am not around a lot any more, but there will be maintainers in the future and it'd be great if they don't have to reverse engineer your intent here.

@saghul is right, there are a few style nits but nothing that takes more than 5 minutes to fix.

so lgtm
@saghul feel free to land this when you think it's good.

saghul · 2014-06-18T12:39:53Z

src/win/pipe.c

@@ -1771,8 +1769,33 @@ static void eof_timer_close_cb(uv_handle_t* handle) {


 int uv_pipe_open(uv_pipe_t* pipe, uv_file file) {
-  HANDLE os_handle = uv__get_osfhandle(file);
-  DWORD duplex_flags = UV_HANDLE_READABLE | UV_HANDLE_WRITABLE;
+  HANDLE os_handle = (HANDLE)uv__get_osfhandle(file);


Unneeded cast?

saghul · 2014-06-18T12:41:56Z

Thanks @piscisaureus! I made some comments about the style issues, @Keno @vtjnash please fix those and add a explanatory comment and this is good to go!

saghul · 2014-07-06T18:08:09Z

Made the changes myself and landed it in ebafb90, thanks!

vtjnash referenced this pull request in JuliaAttic/libuv-archive Jan 2, 2014

fix compatibility with cygwin pipes: be more tolerant of the pipes co…

866b38b

…nnected. also, duplicate handles to ensure the original handle remains valid after uv_close (esp. important for stdio)

vtjnash reviewed May 15, 2014
View reviewed changes

windows: fix compatibility with cygwin pipes

1158e4c

This makes libuv more tolerant of the properties of the pipes that it can use without any issue. this is necessary because Cygwin (and hence Mintty) opens STDIN without FILE_WRITE_ATTRIBUTES.

saghul reviewed Jun 18, 2014
View reviewed changes

saghul closed this Jul 6, 2014

vtjnash deleted the jn/cygwin_pipes branch May 26, 2016 02:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows: fix compatibility with cygwin pipes #1067

windows: fix compatibility with cygwin pipes #1067

vtjnash commented Jan 2, 2014

Nodejs-Jenkins commented Jan 2, 2014

txdv commented Jan 2, 2014

vtjnash commented Jan 3, 2014

saghul commented Feb 7, 2014

Keno commented Feb 22, 2014

oberlies commented May 14, 2014

saghul commented May 14, 2014

piscisaureus commented May 14, 2014

vtjnash May 15, 2014

vtjnash commented May 15, 2014

vtjnash May 15, 2014

saghul Jun 18, 2014

Keno commented Jun 3, 2014

Keno commented Jun 11, 2014

saghul commented Jun 18, 2014

piscisaureus commented Jun 18, 2014

saghul Jun 18, 2014

saghul commented Jun 18, 2014

saghul commented Jul 6, 2014

windows: fix compatibility with cygwin pipes #1067

windows: fix compatibility with cygwin pipes #1067

Conversation

vtjnash commented Jan 2, 2014

Nodejs-Jenkins commented Jan 2, 2014

txdv commented Jan 2, 2014

vtjnash commented Jan 3, 2014

saghul commented Feb 7, 2014

Keno commented Feb 22, 2014

oberlies commented May 14, 2014

saghul commented May 14, 2014

piscisaureus commented May 14, 2014

vtjnash May 15, 2014

Choose a reason for hiding this comment

vtjnash commented May 15, 2014

vtjnash May 15, 2014

Choose a reason for hiding this comment

saghul Jun 18, 2014

Choose a reason for hiding this comment

Keno commented Jun 3, 2014

Keno commented Jun 11, 2014

saghul commented Jun 18, 2014

piscisaureus commented Jun 18, 2014

saghul Jun 18, 2014

Choose a reason for hiding this comment

saghul commented Jun 18, 2014

saghul commented Jul 6, 2014