Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

readline treats \n\r as two lines, passes content through #3305

Closed
jhormuz opened this issue May 22, 2012 · 9 comments
Closed

readline treats \n\r as two lines, passes content through #3305

jhormuz opened this issue May 22, 2012 · 9 comments
Labels

Comments

@jhormuz
Copy link

jhormuz commented May 22, 2012

This is really two bugs, but they are in the same bit of code.

When I write "cat" with nodejs, it sends the input to the stdio twice. It also treats \n\r as two lines. This only happens when piping data through the program, at the command line things work.

Here is some sample code:


!/usr/bin/node

var readline = require('readline'),
rl = readline.createInterface(process.stdin, process.stdout),

prefix=""

rl.on('line', function(line) {
console.log("line is [" + line + "]")
}).on('close', function() {
process.exit(0);
});


At the command line, things work:

first line
line is [first line]
second line
line is [second line]

but when I pipe through a file with Unix style return it does the following:

echo -e "first line\nsecond line" | ./sample.js
first line <===== extra
line is [first line]
second line <===== extra
line is [second line]

and when I pipe through something with the \r added, it does the following:

echo -e -n "first line\n\rsecond line\n\r" | ./sample.js
first line <===== extra
line is [first line]
<===== extra
line is [] <===== extra
second line <===== extra
line is [second line]
<===== extra
line is [] <===== extra

This makes it kind of, um, impossible to use node.js as a command line tool, which is unfortunate, because we have to process pages of JSON data.

I'd be happy to personally debug this and get you guys a patch, if you think you might use it.

@bnoordhuis
Copy link
Member

\n\r is not an end-of-line convention on any platform we support. Try \r\n.

@rlidwka
Copy link

rlidwka commented May 22, 2012

Node 0.6.x detects if it's user input looking only to stdout, not stdin. So echo 'test' | node test.js wont work but echo 'test' | node test.js | cat will. I doubt that this behaviour is going to be changed in a stable branch.

In node 0.7.x it seems that you should manually set isterminal flag like that:

#!/usr/bin/node

var readline = require('readline'),
rl = readline.createInterface(process.stdin, process.stdout, undefined, 
   process.stdin.isTTY && process.stdout.isTTY),

prefix=""

rl.on('line', function(line) {
console.log("line is [" + line + "]")
}).on('close', function() {
process.exit(0);
});

PS: well yeah... there are a few bugs around there :)

// @TooTallNate: fixed

@TooTallNate
Copy link

@rlidwka @jhormuz Don't use tty.isatty() (that function expects a fd, not a Stream anyways). Check process.stdin.isTTY and process.stdout.isTTY instead.

@clausreinke
Copy link

any ideas about the '\r\n' part? lib/readline.js suggests correct split pattern (unless buffer ends between '\r\n') but testing still shows incorrect splits

$ cat rl-test.script
require("readline")
    .createInterface({input:process.stdin,output:process.stdout,terminal:false})
    .on("line",function(l){ console.log("|"+l+"|") });

$ echo -e -n "1\r\n2\r\n" | node rl-test.script
|1
|
|2
|

$ node --version
v0.8.15

@clausreinke
Copy link

This line in readline.js looks suspicious, trying to identify line endings based on single-character comparisons alone.

bnoordhuis added a commit to bnoordhuis/node that referenced this issue Jan 28, 2013
Make lines ending \r\n emit one 'line' event, not two (where the second
one is an empty string).

This adds a new keypress name: 'return' (as in: 'carriage return').

Fixes nodejs#3305.
@clausreinke
Copy link

Thanks for looking into this. If I understand the pull request correctly, it "disarms" \r as line ending. Aren't there systems that use isolated \r as line endings? If so, the solution would have be context-/history-sensitive (equivalent to split(/\r\n|\n|\r/), but still working if the \r\n is spread over two buffer fills).

Meanwhile, workarounds for in-production node versions look complicated, especially as they do not have the new Streams yet (I was thinking of replacing \r\n with \n in stdin before passing it to readline).

@isaacs
Copy link

isaacs commented Jan 29, 2013

Aren't there systems that use isolated \r as line endings?

Node does not support pre-OS X Macintosh systems. (Neither do most Mac OS X programs these days.) I think we're ok there.

@bnoordhuis
Copy link
Member

Fixed in 9bd9c54.

@isaacs
Copy link

isaacs commented Jan 30, 2013

The problem here is that the repl is busted by 9bd9c54. (I guess we all ran make test and forgot to actually try using node, whoops.)

I think when you're reading a stream, \r should behave like home, but when you're writing a \r should behave like a \n, since otherwise the repl doesn't work in TTY mode.

@isaacs isaacs reopened this Jan 30, 2013
isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jan 30, 2013
@isaacs isaacs closed this as completed in 60f18ed Jan 30, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants