Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: readline on "Git Bash" - winpty required #14100

Closed
starkwang opened this issue Jul 6, 2017 · 14 comments
Closed

doc: readline on "Git Bash" - winpty required #14100

starkwang opened this issue Jul 6, 2017 · 14 comments
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. wip Issues and PRs that are still a work in progress. wsl Issues and PRs related to the Windows Subsystem for Linux.

Comments

@starkwang
Copy link
Contributor

starkwang commented Jul 6, 2017

  • Version: master
  • Platform: Windows7, Git Bash
  • Subsystem: readline

On Windows7, the Node.js application don't terminate after rl.close() :

const readline = require('readline');
const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});
rl.close();

It causes test/parallel/test-readline-interface.js TIMEOUT, which will not happen in v6.x.x

@starkwang starkwang changed the title readline: Process don't terminate after readline.close() readline: process don't terminate after readline.close() Jul 6, 2017
@mscdex mscdex added readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. labels Jul 6, 2017
@vsemozhetbyt
Copy link
Contributor

FWIW, I can not reproduce the issue on Windows with v4, v6, v8 or v9.

@starkwang
Copy link
Contributor Author

This only happens in Git Bash.
In PowerShell or cmd, It's fine.

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/platform-windows, @addaleax, @Fishrock123

@tniessen
Copy link
Member

tniessen commented Jul 6, 2017

I can repoduce this. A similar and probably related problem occurs when executing node.exe without arguments, which should normally start the built-in repl, but does not under Git Bash. node.exe -i starts the repl, so I assume the console / tty configuration does not work correctly in Git Bash.

@vsemozhetbyt
Copy link
Contributor

Someone told me Node.js supported building only with cmd.exe on Windows. I am not sure if we support test runs in other shells or work in other shells at all(

@joaocgreis
Copy link
Member

This looks like a duplicate of #5620 .

Please always use winpty when running Node on Git Bash. Actually, if you run just node, Git Bash will add winpty for you. But if you run node.exe, a different file name or with a path, you have to use winpty. Try:

winpty node.exe script.js

@refack refack added the doc Issues and PRs related to the documentations. label Jul 6, 2017
@refack
Copy link
Contributor

refack commented Jul 6, 2017

Please always use winpty when running Node on Git Bash.

So I'd suggest we document this, but also disclaim official support (Experimental / YMMV) for: Git Bash / MSYS / MinGW / Cygwin / WSL

@refack refack changed the title readline: process don't terminate after readline.close() doc: readline on "Git Bash" - process don't terminate after readline.close() - winpty required Jul 6, 2017
@refack refack changed the title doc: readline on "Git Bash" - process don't terminate after readline.close() - winpty required doc: readline on "Git Bash" - process doesn't terminate after readline.close() - winpty required Jul 6, 2017
@refack refack changed the title doc: readline on "Git Bash" - process doesn't terminate after readline.close() - winpty required doc: readline on "Git Bash" - winpty required Jul 6, 2017
@seishun
Copy link
Contributor

seishun commented Jul 6, 2017

Your test case is effectively:

process.stdin.resume();
process.stdin.pause();

When running in Git Bash, GetFileType on stdin returns FILE_TYPE_PIPE rather than FILE_TYPE_CHAR. Consequently, Node.js doesn't consider it a TTY and sets its highWaterMark to the default value of 16384, rather than 0 as in cmd.

Once you're resumed a stream, it will read until it has read highWaterMark bytes or EOF. Pausing only takes action after a chunk is read. So in your case, the process will exit once you enter 16384 bytes or EOF.

I say we close this since Git Bash is not supported. We can create a separate issue for documentation.

@refack
Copy link
Contributor

refack commented Jul 6, 2017

I say we close this since Git Bash is not supported. We can create a separate issue for documentation.

I kinda already turned this to a documentation issue 😊, I say keep this open...

@refack refack added wsl Issues and PRs related to the Windows Subsystem for Linux. good first issue Issues that are suitable for first-time contributors. labels Jul 22, 2017
@yamalight
Copy link
Contributor

@refack @starkwang I could document that. Can you please point me as to where that would need to be added?

@yamalight
Copy link
Contributor

The only place that mentions platform support seems to be node/deps/uv/SUPPORTED_PLATFORMS.md - would that be the right place to add this info?

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

@yamalight the place we document our platform support is here: https://github.com/nodejs/node/blob/master/BUILDING.md#supported-platforms-1

Not sure what @refack had in mind, but I'd assume something like adding a note2 to the Windows section to say that running through cmd is supported, but running through any other shell is experimental (and then mention winpty for Git Bash and Cygwin).

@yamalight
Copy link
Contributor

@gibfahn thanks, got it. Will make a PR soon 👍

@yamalight
Copy link
Contributor

PR created. Please let me know if you want any changes.

@refack refack added wip Issues and PRs that are still a work in progress. and removed good first issue Issues that are suitable for first-time contributors. labels Oct 10, 2017
@refack refack closed this as completed in 641ba5e Oct 12, 2017
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 15, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: nodejs/node#16104
Fixes: nodejs/node#14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this issue Oct 18, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: #16104
Fixes: #14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this issue Oct 18, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: #16104
Fixes: #14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this issue Nov 16, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: #16104
Fixes: #14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this issue Nov 21, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: #16104
Fixes: #14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this issue Nov 28, 2017
explain that Git Bash and Cygwin require winpty to work correctly.
Added info on Git Bash running winpty automatically
when running node without .exe extension.

PR-URL: #16104
Fixes: #14100
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. wip Issues and PRs that are still a work in progress. wsl Issues and PRs related to the Windows Subsystem for Linux.
Projects
None yet
Development

No branches or pull requests

9 participants