Skip to content

tty: stdio properties are undefined inside exec() child #2333

Description

@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: sindresorhus/grunt-shell#95 raineorshine/npm-check-updates#119

Activity

  1. added
    child_processIssues and PRs related to the child_process subsystem.
    ttyIssues and PRs related to the tty subsystem.
    on Aug 9, 2015
  2. thefourtheye commented on Aug 9, 2015

    @thefourtheye
    Contributor

    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.

  3. silverwind commented on Aug 9, 2015

    @silverwind
    ContributorAuthor

    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.

  4. thefourtheye commented on Aug 9, 2015

    @thefourtheye
    Contributor

    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.

  5. silverwind commented on Aug 9, 2015

    @silverwind
    ContributorAuthor

    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.

  6. thefourtheye commented on Aug 9, 2015

    @thefourtheye
    Contributor

    @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?

  7. silverwind commented on Aug 9, 2015

    @silverwind
    ContributorAuthor

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

  8. thefourtheye commented on Aug 9, 2015

    @thefourtheye
    Contributor

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

  9. silverwind commented on Aug 9, 2015

    @silverwind
    ContributorAuthor

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

  10. brendanashworth commented on Aug 10, 2015

    @brendanashworth
    Contributor

    Mmm #2160?

  11. 14 remaining items

  12. silverwind commented on Jul 12, 2016

    @silverwind
    ContributorAuthor

    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.

  13. removed
    child_processIssues and PRs related to the child_process subsystem.
    on Jul 12, 2016
  14. kaicataldo commented on Jun 4, 2017

    @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!

  15. debugpai commented on Jan 12, 2018

    @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

  16. removed
    good first issueIssues that are suitable for first-time contributors.
    on Mar 23, 2018
  17. gireeshpunathil commented on May 23, 2018

    @gireeshpunathil
    Member

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

  18. kaicataldo commented on May 23, 2018

    @kaicataldo

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

  19. debugpai commented on May 24, 2018

    @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.

  20. kaicataldo commented on May 24, 2018

    @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!

  21. gireeshpunathil commented on May 24, 2018

    @gireeshpunathil
    Member

    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.

  22. bnoordhuis commented on May 25, 2018

    @bnoordhuis
    Member

    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.

  23. gireeshpunathil commented on May 25, 2018

    @gireeshpunathil
    Member

    @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.

  24. bnoordhuis commented on May 25, 2018

    @bnoordhuis
    Member

    Yes, the status quo.

  25. gireeshpunathil commented on Jun 1, 2018

    @gireeshpunathil
    Member

    closing based on the above discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    docIssues and PRs related to Node.js documentation.ttyIssues and PRs related to the tty subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions