tty: stdio properties are undefined inside exec() child

[silverwind]

I'm wondering if this is intentional or a bug:

parent.js

var exec = require("child_process").exec;

exec("iojs child.js", function (err, stdout, stderr) {
  process.stdout.write(stdout);
});

child.js

console.log(process.stdin.isTTY);
console.log(process.stdin.isRaw);
console.log(process.stdin.setRawMode);
console.log(process.stdout.columns);

output:

undefined
undefined
undefined
undefined

ref: https://github.com/sindresorhus/grunt-shell/issues/95 https://github.com/tjunnone/npm-check-updates/issues/119

[thefourtheye]

When the new process is created, the process.stdin is actually a PIPE and it doesn't have isTTY, isRaw and setRawMode defined. We can actually fix them like this, I think

diff --git a/src/node.js b/src/node.js
index af66b17..87121bc 100644
--- a/src/node.js
+++ b/src/node.js
@@ -717,6 +717,12 @@
         stdin._handle.readStop();
       });

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

process.stdout object is actually a Socket object, so columns and rows will not make sense in this case.

[silverwind]

Doing this unconditionally worries me a bit. For the isTTY case, I wonder why the code path in tty_wrap.cc that sets it isn't triggered.

[thefourtheye]

The isTTY is set in tty module's ReadStream constructor function. When the new process is created, guessHandleType calls uv_guess_handle with fd as 0 and than returns PIPE, instead of TTY. So, a net.Socket is created and returned. If uv_guess_handle returned TTY, tty.ReadStream would have been invoked, and isTTY and isRaw would be set properly.

[silverwind]

Hmm, I don't see the process.stdin getter invoked at all when accessing these properties, so your patch doesn't seem to change anything. I'm guessing we have to check out ttywrap.

[thefourtheye]

@silverwind Actually, I tested the changes and it is working fine. Shall I submit a PR with the code in this issue as a test?

[silverwind]

Go ahead, it's good to have a PR so we can run CI anyways.

[thefourtheye]

@silverwind I am not making changes for the columns and rows, as they will be undefined anyway.

[silverwind]

I'm primarily after stdin.isTTY, but I wasn't seeing any difference, it looked as if startup.processStdio was never invoked.

[brendanashworth]

Mmm https://github.com/nodejs/io.js/issues/2160?

[silverwind]

@sabrehagen is it possible that your script was being piped to or was being called by exec when you observed that issue?

[silverwind]

Been thinking about this some more. The docs are pretty clear that these ReadStream properties are only defined when an actual TTY is attached:

When io.js detects that it is being run inside a TTY context, then process.stdin will be a tty.ReadStream

I think I'll just leave it at that. A possible change to introduce these properties would be a breaking change if done right, and I don't think it's worth doing so, it doesn't really solve any issues.

[thefourtheye]

But stdin will be inconsistent when we exec, I am not sure if that is okay

[silverwind]

True. I don't understand completely what exec really does yet. When doing process.stdin.pipe(child.stdin) after exec, I don't think isTTY and isRaw do update correctly, but probably should.

[thefourtheye]

I believe the same problem should be with spawn as well. I'll check that today

[silverwind]

I gave up on doing the rework of your PR because there'd be a tiny change of breakage when someone does process.stdin.isTTY === undefined and it's kind of a pointless breaking change if we intialize these values when stdin isn't actually a ReadStream.

The issue I was trying to solve is this:

var child = exec("iojs child.js", function (err, stdout, stderr) {
  process.stdout.write(stdout);
});
process.stdin.pipe(child.stdin);

After this pipe is made, the child's stdin should upgrade to a ReadStream instance (and have these properties defined). Ideally, this should happen before the child executes its first line of code.

[Trott]

@silverwind wrote:

Ideally, this should happen before the child executes its first line of code.

The child is a separate node process, right? Is there realistically even a way to guarantee order like that when there are two separate processes?

[59naga]

FYI when i encountered this issue, wanted the following requirements:

• execution of a command that can't be run using spawn.
• non-blocking (not sync)
• stdio inheritance

In other words, by executing a shell in the spawn, it has been resolved.

// parent.js
const spawn = require('child_process').spawn;
spawn('NODE_ENV=production node ./child.js', { shell: true, stdio: 'inherit' });

// child.js
const chalk = require('chalk');
process.stdout.write(`${chalk.magenta('child')}\n`);

the above doesn't work with node 5.6 or less

[jasnell]

I'm wondering if there's really anything to do here other than simply expand the documentation to account for this.

[Fishrock123]

@silverwind is this a problem? If you spawn a child process it doesn't have a direct connection to a TTY and so stdio will not be TTY streams.

Marking for docs.

[silverwind]

It irks me that properties that are meant to be boolean are returning undefined, but in most cases it won't matter because user code will make use of implicit Boolean(undefined) === false. Still I think a patch to initialize isTTY and isRaw to false would not hurt for the sake of correctness.

Overall, the TTY docs could certainly be improved, for example the isTTY boolean isn't properly documented, and I think a link from the process properties to the TTY docs would certainly clear up the fact that stdio streams have special properties.

[kaicataldo]

Still I think a patch to initialize isTTY and isRaw to false would not hurt for the sake of correctness.

Is this still an issue? Looking to contribute and it looks like both are initialized to a boolean value (though isTTY is initialized to true). Thanks!

[debugpai]

Can some of the maintainers have a look at this thread please? I was looking to pick it up too but there's no data if its still required or not

[gireeshpunathil]

@debugpai2 - are you still interested to take this up?

[kaicataldo]

If @debugpai2 isn't, I am! Let me see if I can take a look this week.

[debugpai]

I am quite busy this month so won't be able to. If anyone else wants to take it up, feel free to do so.

[kaicataldo]

I took a look at the code yesterday and then realized it's not clear to me what the issue is. Is there any chance someone could sum up what the problem/requested change is here? Thanks!

[gireeshpunathil]

The idea is to have meaningful values to the below 4 properties for Node child processes. Right now, they are undefined. A test case that manifests the issue is in the original posting above.

• process.stdin.isTTY
• process.stdin.isRaw
• process.stdin.setRawMode
• process.stdout.columns

Apparently it is a trivial change, but a bit of complexity arises due to the fact plurality of Node child processes can be created through exec, fork and spawn family functions that uses different and tunable mechanisms for stdio creation. So the fix would be to reflect on the process creation and set these values accordingly.

/cc @silverwind if I missed anything.

[bnoordhuis]

I don't know about stubbing .setRawMode(). No meaningful way to implement except for throwing an exception, but that breaks feature detection: if (handle.setRawMode) handle.setRawMode(true).

No meaningful value exists for .columns when the handle isn't a tty, except maybe zero. Not really an improvement.

I think I'd leave well enough alone.

[gireeshpunathil]

@bnoordhuis - agreed on the points you mentioned , but did not follow the last phrase :) - you meant to say you recommend the status quo, right? Just double checking.

[bnoordhuis]

Yes, the status quo.

[gireeshpunathil]

closing based on the above discussion