child_process: set stdin properties when execed

[thefourtheye]

When a child process is created, the stdin will not have isTTY,
isRaw and setRawMode properties. Because, uv_guess_handle in
guessHandleType call returns PIPE for fd 0. So, we create a
net.Socket and return. But normally it will return TTY and we create
tty.ReadStream and return where all those properties are properly set.

This path explicitly sets the above mentioned properties on the returned
socket object.

Fixes: #2333

[thefourtheye]

CI Run: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/60/

[silverwind]

Hmm, I'm just getting 4x undefined still after this patch on OS X. What OS are you on?

[thefourtheye]

@silverwind Oh, I am using Ubuntu 14.04.2

[silverwind]

What does my test case print for you? I'll try this on linux too, building will take a while though.

[thefourtheye]

Your testcase prints undefined four times, one in each line.

[silverwind]

Shouldn't it like print those properties? (parent prints child's stdout, which logs them) :)

[silverwind]

Oh wait I'm dumb. I need to supply the built binary to exec of course :D

[thefourtheye]

@silverwind You meant after the fix? It prints true, false, [Function setRawMode] and undefined

[silverwind]

Yeah, works as expected now. Let's see what the CI says.

[thefourtheye]

@silverwind It fails only in Windows, as of now, but that is because the splitting with os.EOL. I ll change it to split with \n only.

[silverwind]

Also breaks the REPL it seems.

[thefourtheye]

Oh, which OS? I couldn't see any REPL failures till now.

[silverwind]

OS X:

$ ./iojs
node.js:723
        throw new Error('Not a raw device');
        ^

Error: Not a raw device
    at ReadStream.setRawMode (node.js:723:15)
    at REPLServer.Interface._setRawMode (readline.js:173:23)
    at REPLServer.Interface (readline.js:133:10)
    at new REPLServer (repl.js:229:16)
    at Object.exports.start (repl.js:467:14)
    at Object.createRepl [as createInternalRepl] (internal/repl.js:60:21)
    at startup (node.js:127:19)
    at node.js:958:3

[thefourtheye]

Ah, it breaks in Ubuntu 14.04.02 as well. Till now they were checking if setRawMode is a function or not, before calling it, in readline

Interface.prototype._setRawMode = function(mode) {
  if (typeof this.input.setRawMode === 'function') {
    return this.input.setRawMode(mode);
  }
};

Should this be considered as a breaking change and the repl should also be fixed? Because, Socket objects can not be considered as raw devices AFAIK.

[silverwind]

You could wrap in a try instead of checking for a function for the REPL.

I'm not understanding yet why you call it a Socket. Isn't stdin basically just a readable stream from fd 0?

[thefourtheye]

Not if tty_wrap.guessHandleType returns PIPE or NET. We return a net.Socket instance in those cases.

[silverwind]

Hmm, wouldn't it be better to set the missing properties just in the PIPE case then? Seems to be safer to me.

[silverwind]

Something like this maybe:

diff --git a/src/node.js b/src/node.js
index dfe3599..347d424 100644
--- a/src/node.js
+++ b/src/node.js
@@ -649,8 +649,9 @@

       var tty_wrap = process.binding('tty_wrap');
       var fd = 0;
+      var handleType = tty_wrap.guessHandleType(fd);

-      switch (tty_wrap.guessHandleType(fd)) {
+      switch (handleType) {
         case 'TTY':
           var tty = NativeModule.require('tty');
           stdin = new tty.ReadStream(fd, {
@@ -717,11 +718,14 @@
         stdin._handle.readStop();
       });

-      stdin.isTTY = true;
-      stdin.isRaw = false;
-      stdin.setRawMode = function setRawMode() {
-        throw new Error('Not a raw device');
-      };
+      // set the missing TTY properties when spawned through child_process.exec
+      if (handleType === 'PIPE') {
+        stdin.isTTY = true;
+        stdin.isRaw = false;
+        stdin.setRawMode = function setRawMode() {
+          throw new Error('Not a raw device');
+        };
+      }

       return stdin;
     });

[thefourtheye]

But the properties will be still missing for the NET case, right?

[silverwind]

Yeah. I'm not even sure what a NET handle would be. A quick look at uv_guess_handle doesn't give an impression it exists. Also I think we should actually set isTTY to false in the PIPE case.

[silverwind]

I think we're confusing something. It's 'TCP' we're talking about. Anyway, it doesn't look like uv_guess_handle is able to return UV_TCP either, so I think it can safely be removed from the switch statement. It was added in e99dff4.

[thefourtheye]

Hmmm I am on mobile now. I'll also go through the code once, with your inputs, in the morning.

[silverwind]

Sure, here's what I propose:

diff --git a/src/node.js b/src/node.js
index dfe3599..7fe4c48 100644
--- a/src/node.js
+++ b/src/node.js
@@ -666,7 +666,6 @@
           break;

         case 'PIPE':
-        case 'TCP':
           var net = NativeModule.require('net');

           // It could be that process has been started with an IPC channel
@@ -688,6 +687,13 @@
           }
           // Make sure the stdin can't be `.end()`-ed
           stdin._writableState.ended = true;
+
+          // Add missing TTY properties
+          stdin.isTTY = false;
+          stdin.isRaw = false;
+          stdin.setRawMode = function setRawMode() {
+            throw new Error('Not a raw device');
+          };
           break;

         default:
@@ -717,12 +723,6 @@
         stdin._handle.readStop();
       });

-      stdin.isTTY = true;
-      stdin.isRaw = false;
-      stdin.setRawMode = function setRawMode() {
-        throw new Error('Not a raw device');
-      };
-
       return stdin;
     });

[silverwind]

I'm pondering if it's actually worth to do the setRawMode thing after all, it's possibly breaking as we've seen. isTTY and isRaw are somewhat worthy correctness changes, thought they don't really fix the issue described in sindresorhus/grunt-shell#95 (comment), but that was expected :)

[bnoordhuis]

This looks incorrect to me. .isTTY should only be true when stdin refers to a real tty, not when it's a file/pipe/socket/etc.

You could wrap it in a if (!process.stdin.hasOwnProperty('isTTY')) { ... } block, although that feels kind of icky (and the fact that .isTTY is an own property is something of an implementation detail, so if (!('isTTY' in process.stdin)) { .. } is probably more correct.)

[silverwind]

Yeah, stdin.isTTY should definitely be false in the PIPE case. I corrected that in my latest diff suggestion.

The whole change is more of a correctness thing. Most scripts do just if (stdin.isTTY), in which case it wouldn't matter if it's undefined or false.

[jbergstroem]

The FreeBSD machine times out, not sure if relevant (can't check right now).

[silverwind]

@thefourtheye I'll do a PR which supersedes this one if you don't mind.

[thefourtheye]

@silverwind Oh no problem :-) I ll close this.

[rafaelcastrocouto]

Hey guys, we are having an error at devtool related to stdin that only happens in windows Experience-Monks/devtool#42 and I have no clue how to solve it. Seems like electron "renderer" process.stdin is throwing the Exception: Error: Implement me. Unknown stdin file type! error. Can anyone help? Thx a lot!

[bnoordhuis]

@rafaelcastrocouto How about you file a new issue instead of hijacking an existing pull request?

[rafaelcastrocouto]

sorry @bnoordhuis ... in my defense I just wanted some advice and I really don't think this is a node issue. Since the subject of this issue seems very related to my problem I just thought someone could easily point how to solve it.

I'll keep looking ... thx and sorry again

[bnoordhuis]

@rafaelcastrocouto If you have a general support question, try https://github.com/nodejs/help/issues. This issue tracker is pretty high-volume, that's why we try to keep it focused.

[rafaelcastrocouto]

Thx a lot @bnoordhuis ... nodejs/help#105