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

Input from TTY is strange #2504

Closed
anseki opened this issue Aug 23, 2015 · 16 comments
Closed

Input from TTY is strange #2504

anseki opened this issue Aug 23, 2015 · 16 comments
Labels
readline Issues and PRs related to the built-in readline module. stream Issues and PRs related to the stream subsystem. windows Issues and PRs related to the Windows platform.

Comments

@anseki
Copy link

anseki commented Aug 23, 2015

I tried read from STDIN by fs.readSync that is given process.stdin.fd.
For example:

var
  fs = require('fs'),
  buffer = new Buffer(1024),
  fd = process.platform === 'win32' ? process.stdin.fd : fs.openSync('/dev/tty', 'r'),
  readSize = fs.readSync(fd, buffer, 0, 1024);

console.log('INPUT: ' + buffer.toString('utf8', 0, readSize));

In v2.3.1-, no problem:

foo
INPUT: foo

In v2.3.2+, first line is ignored, and second line is accepted:

foo
bar
INPUT: bar

This problem occurs in only Windows + iojs v2.3.2+.
I don't use process.stdin.fd in non-Win because fs.readSync can't read it.
Therefore, process.stdin of v2.3.2+ might have problem. (not fs.readSync)

@brendanashworth brendanashworth added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Aug 23, 2015
@brendanashworth
Copy link
Contributor

This is seems to be a regression that could have been introduced by 8cee8f5 (didn't do git bisect, just layman's bisecting). @nodejs/platform-windows ?

@anseki
Copy link
Author

anseki commented Aug 23, 2015

If 8cee8f5 made this problem, I think that windows label is not suitably, because I didn't use process.stdin in non-Win. And in case that doesn't use process.stdin (open('CONIN$') instead of it) in Windows is no problem.

@mscdex
Copy link
Contributor

mscdex commented Aug 23, 2015

From what I can tell it has to do with process.stdin taking so long to initialize when first accessed.

For example, if you use:

process.stdin;
console.log('ready');
var
  fs = require('fs'),
  buffer = new Buffer(1024),
  fd = process.platform === 'win32' ? process.stdin.fd : fs.openSync('/dev/tty', 'r'),
  readSize = fs.readSync(fd, buffer, 0, 1024);

console.log('INPUT: ' + buffer.toString('utf8', 0, readSize));

Then the first line will get read as expected if you type it after ready is displayed.

@mscdex
Copy link
Contributor

mscdex commented Aug 23, 2015

You can also replace process.stdin.fd in the original code with just 0 and it will get the first line too.

I'm trying this all on Windows, so I haven't looked at it on *nix. Also, reverting 8cee8f5 does not affect this particular issue.

@anseki
Copy link
Author

anseki commented Aug 24, 2015

Thank you, but... I tried your solution, and it did not read first line.

ready
foo
bar
INPUT: bar

I tried your sample code on Windows 8.1 64bit with iojs v2.3.1, v2.3.2 and v3.1.0. Of course v2.3.1 works fine.

Also I tried 0 instead of process.stdin.fd. It works fine before accessing to process.stdin, but it doesn't read first line after accessing to process.stdin.

For example, this code doesn't read first line:

var
  fs = require('fs'),
  buffer = new Buffer(1024),
  fd = process.platform === 'win32' ? process.stdin.fd : fs.openSync('/dev/tty', 'r'),
  //readSize = fs.readSync(fd, buffer, 0, 1024);
  readSize = fs.readSync(0, buffer, 0, 1024);

console.log('INPUT: ' + buffer.toString('utf8', 0, readSize));

Yes I know that fd is meaningless now, then this works fine:

var
  fs = require('fs'),
  buffer = new Buffer(1024),
  //fd = process.platform === 'win32' ? process.stdin.fd : fs.openSync('/dev/tty', 'r'),
  //readSize = fs.readSync(fd, buffer, 0, 1024);
  readSize = fs.readSync(0, buffer, 0, 1024);

console.log('INPUT: ' + buffer.toString('utf8', 0, readSize));

But process.stdin might be used at a place I don't know. (e.g. other packages)
That is, this code doesn't read first line:

process.stdin;

var
  fs = require('fs'),
  buffer = new Buffer(1024),
  readSize = fs.readSync(0, buffer, 0, 1024);

console.log('INPUT: ' + buffer.toString('utf8', 0, readSize));

I think that a point is that the programers don't know that process.stdin makes problem, and it should not so. (this is not designed behavior, right?)

@orangemocha
Copy link
Contributor

@nodejs/streams : is it legal to use fs.readSync to read from process.stdin, which in this case is not a file?

@calvinmetcalf
Copy link
Contributor

so is the issue that references to process.stdin initiate the readable stream, which does some buffering and that buffering prevents somebody from reading stdin like it was a file?

@anseki
Copy link
Author

anseki commented Sep 9, 2015

It seems that programmers use read/readSync with STDIN to get the user's input easily instead of process.stdin.on('data'). Also my module readlineSync tries readSync.
https://www.google.com/search?q=nodejs+read+from+stdin

This problem occurs when it reads from TTY. It seems no problem when it reads from the file or pipe. (e.g. script < file, echo foo | script etc)

@anseki
Copy link
Author

anseki commented Oct 17, 2015

It seems that the Readline module also is affected by this problem.

For example:

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

readline.question('> ', function(answer) {
  console.log('INPUT: ', answer);
  readline.close();
});

In io.js v2.3.1-, no problem:

> foo
INPUT:  foo

In io.js v2.3.2+, strange second line is shown:

> foo
foo
INPUT:  foo

Node.js v4.2.1 also returns same result:

> foo
foo
INPUT:  foo

@anseki
Copy link
Author

anseki commented Oct 17, 2015

It seems that REPL also is affected by this problem.

I typed a first 1 and pushed the Enter key.
In io.js v2.3.1-, no problem:

E:\test>node
> 1
1
>

In io.js v2.3.2+, strange second 1 is shown:

E:\test>node
> 1
1
1
>

Node.js v4.2.1 also returns same result:

E:\test>node
> 1
1
1
>

@anseki anseki changed the title fs.readSync of v2.3.2+ can't read first line from STDIN Input from STDIN is strange Oct 17, 2015
@anseki anseki changed the title Input from STDIN is strange Input from TTY is strange Oct 17, 2015
@anseki
Copy link
Author

anseki commented Oct 17, 2015

It seems that only the first line of both readline module and REPL is strange, like readSync.
#2504 (comment)

For example:

> foo
foo
INPUT:  foo
> bar
INPUT:  bar
E:\test>node
> 1
1
1
> 2
2
>

@silverwind silverwind added readline Issues and PRs related to the built-in readline module. stream Issues and PRs related to the stream subsystem. and removed fs Issues and PRs related to the fs subsystem / file system. labels Oct 17, 2015
@piscisaureus
Copy link
Contributor

I think you're looking at a limitation of the windows console.

Once a line-buffered read is started, it cannot be interrupted. So possibly a recent patch changed streams behavior such that data flow from stdin is started, then paused; since the line-buffered read can't be cancelled, it needs to complete first before readSync(0, ...) can read data.

@anseki
Copy link
Author

anseki commented Oct 17, 2015

Thanks piscisaureus, but, sorry, my English is poor. 😓
Do you mean that new Buffer(1024) is problem?
Is these behavior not bug?

@anseki
Copy link
Author

anseki commented Oct 18, 2015

I confirmed that the first code of this issue #2504 (comment) (i.e. fs.readSync) in v4.2.1 also returns same result.

And it seems that fs.read also is affected.

For example:

var
  fs = require('fs'),
  buffer = new Buffer(1024),
  fd = process.stdin.fd;

fs.read(fd, buffer, 0, 1024, null, function(err, bytesRead, buffer) {
  if (err) { throw err; }
  console.log('INPUT: ' + buffer.toString('utf8', 0, bytesRead));
});

In v2.3.1:

foo
INPUT: foo

In v2.3.2:

foo
bar
INPUT: bar

In v4.2.1:

foo
bar
INPUT: bar

Till now, I confirmed issue in:

  • fs.readSync
  • fs.read
  • readline
  • REPL

@silverwind
Copy link
Contributor

Filed #3490 for the revert. I could use some assistence with a regression test for this.

silverwind added a commit to silverwind/node that referenced this issue Oct 26, 2015
This reverts 8cee8f5 which was causing a regression through a possible
race condition on Windows 8 and 10.

Fixes: nodejs#2996
Fixes: nodejs#2504
rvagg pushed a commit to rvagg/io.js that referenced this issue Nov 7, 2015
This reverts 8cee8f5 which was causing a regression through a possible
race condition on Windows 8 and 10.

Fixes: nodejs#2996
Fixes: nodejs#2504
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. stream Issues and PRs related to the stream subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants