Repository navigation
tty: stdio properties are undefined inside exec() child #2333
Description
Activity
- addedchild_processIssues and PRs related to the child_process subsystem.Issues and PRs related to the child_process subsystem.ttyIssues and PRs related to the tty subsystem.Issues and PRs related to the tty subsystem.
on Aug 9, 2015 When the new process is created, the
process.stdinis actually a PIPE and it doesn't haveisTTY,isRawandsetRawModedefined. We can actually fix them like this, I thinkdiff --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.stdoutobject is actually aSocketobject, socolumnsandrowswill not make sense in this case.Doing this unconditionally worries me a bit. For the isTTY case, I wonder why the code path in
tty_wrap.ccthat sets it isn't triggered.The
isTTYis set inttymodule'sReadStreamconstructor function. When the new process is created,guessHandleTypecallsuv_guess_handlewithfdas0and than returnsPIPE, instead ofTTY. So, anet.Socketis created and returned. Ifuv_guess_handlereturnedTTY,tty.ReadStreamwould have been invoked, andisTTYandisRawwould be set properly.Hmm, I don't see the
process.stdingetter 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.@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?
Go ahead, it's good to have a PR so we can run CI anyways.
@silverwind I am not making changes for the
columnsandrows, as they will beundefinedanyway.I'm primarily after
stdin.isTTY, but I wasn't seeing any difference, it looked as ifstartup.processStdiowas never invoked.Mmm #2160?
14 remaining items
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 implicitBoolean(undefined) === false. Still I think a patch to initializeisTTYandisRawtofalsewould not hurt for the sake of correctness.Overall, the TTY docs could certainly be improved, for example the
isTTYboolean isn't properly documented, and I think a link from theprocessproperties to the TTY docs would certainly clear up the fact that stdio streams have special properties.Reacted by Chris Gaudreau, getsnoopy and Matthew Gamble- removedchild_processIssues and PRs related to the child_process subsystem.Issues and PRs related to the child_process subsystem.
on Jul 12, 2016 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
isTTYis initialized totrue). Thanks!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
Reacted by Ujjwal Sharma- removedgood first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.
on Mar 23, 2018 @debugpai2 - are you still interested to take this up?
If @debugpai2 isn't, I am! Let me see if I can take a look this week.
Reacted by Gireesh PunathilI am quite busy this month so won't be able to. If anyone else wants to take it up, feel free to do so.
Reacted by Gireesh PunathilI 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!
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,forkandspawnfamily functions that uses different and tunable mechanisms forstdiocreation. So the fix would be to reflect on the process creation and set these values accordingly./cc @silverwind if I missed anything.
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
.columnswhen the handle isn't a tty, except maybe zero. Not really an improvement.I think I'd leave well enough alone.
Reacted by Gireesh Punathil@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.
Yes, the status quo.
closing based on the above discussion
I'm wondering if this is intentional or a bug:
parent.js
child.js
output:
ref: sindresorhus/grunt-shell#95 raineorshine/npm-check-updates#119