Windows: make stdout/sterr pipes blocking #7196
Conversation
One test case in test-stream2-stderr-sync.js was creating a TTY object using an undocumented constructor and passing in fd 2. However, this is running in a child process and fd 2 is actually a pipe, not a TTY. The constructor fails on Windows and causes the handle type to be left uninitialized, which later causes an assert to fail. On Unix, the constructor fails to retrieve the windows size but unlike on Windows, it just leaves the size fields undefined and continues with initializing the stream type, yielding a semi-usable object. I could make the Windows version match Unix behavior, but it seems to me that the test is relying on an implementation detail of an undocumented API, and the Unix behavior is not necessarily more correct than the Windows one. Thus it makes more sense to remove this test.
|
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:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
|
Note that the first commit had already been reviewed here: #6884 |
| @@ -267,6 +267,10 @@ void PipeWrap::Open(const FunctionCallbackInfo<Value>& args) { | |||
|
|
|||
| if (err != 0) | |||
| ThrowException(UVException(err, "uv_pipe_open")); | |||
|
|
|||
| if (args.Length() > 1 && args[1]->IsTrue()) { | |||
| uv_stream_set_blocking((uv_stream_t*)&wrap->handle_, 1); | |||
There was a problem hiding this comment.
We want to check the return value here and do the ThrowException dance as well
|
This looks fine to me, minus the nit about checking the return value |
|
Updated, thanks. |
|
for posterity, we're moving the api to a specific method |
|
This landed in 20176a9 |
|
Question: would be possible/reasonable to backport this to the node v0.10.x branch? The reason I ask is that this fix is needed for log output to show up correctly on http://www.appveyor.com/ and given appveyor is becoming quite popular and node v0.12.x is not out yet, it would be really nice to have this in v0.10.x. Details at mapnik/node-mapnik#257 (comment) on why appveyor needs at least node v0.11.x with this fix for correct log display. |
|
I have no objections. cc @tjfontaine ? |
|
Ping. |
|
Hi guys, are you still planning to backport that into 0.10.x? |
See joyent#3584
I took 0b3021e and modified slightly
to avoid exposing the 'blocking' option. Instead, stdout and stderr
pipes are always implicitly set as blocking.
This fixes test-stream2-stderr-sync.js on Windows.