Why are readline 'line' events fired when you write? #4243

Closed
Niggler opened this Issue Nov 5, 2012 · 9 comments

Projects

None yet

5 participants

@Niggler
Niggler commented Nov 5, 2012

The documentation says

"Emitted whenever the input stream receives a \n, usually received when the user hits enter, or return. This is a good hook to listen for user input."

However the code disagrees:

https://github.com/joyent/node/blob/master/lib/readline.js#L311

(write -> _normalWrite -> _onLine -> emits line event)

Unless I completely misunderstood the code, the write loop seems to trigger line events ...

@TooTallNate

But that loop is only called when the input data contains at least one \n char, so it isn't called for every _normalWrite() call. The code in question should work as documented. Do you have a failing test case?

@Niggler
Niggler commented Nov 5, 2012

All you are saying is that it's only called when you write something that includes a newline character. The documentation says "Emitted whenever the input stream receives a \n", and that's not what's happening here.

As an example:

var readline = require('readline');

var rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout,
  terminal: false
});
var count = 0;
rl.on('line', function (line) {
  rl.write(++count + ':'+line)
});

Save it as test.js and run it

$ seq 1 5 | node test.js
events.js:49
EventEmitter.prototype.emit = function() {
                                  ^
RangeError: Maximum call stack size exceeded

However replacing rl.write with process.stdout.write doesn't cause the error you see

@TooTallNate

Well the "line" argument in the "line" event contains the \n char, so if you call rl.write() with a string containing a newline, then another "line" event will fire, hence the endless loop and call stack being exceeded.

To fix your test case, use trim() to remove the trailing \n. i.e. rl.write(++count + ':'+line.trim()).

@TooTallNate

On a second look though, the fact that the trailing \n is included in the "line" argument might be considered a bug, since it happens only when terminal: false, but not when terminal: true. We should probably normalize that (stripping the newline being the correct behavior IMO).

@piscisaureus
Member

@TooTallNate You know what to do now :-)

@TooTallNate TooTallNate was assigned Nov 5, 2012
@TooTallNate

Assigned to self.

@ackalker
ackalker commented Nov 5, 2012

I don't understand why rl.write(), which should use the output specified in the interface, should emit a 'line' event at all.
How else are we supposed to write line oriented 'Unix filter' style scripts using readline?

To be more clear:
rl.on('line', ...) should handle 'line' event on input stream
rl.write(data) sends data to output stream.
Two different things.

@TooTallNate TooTallNate added a commit to TooTallNate/node that referenced this issue Nov 6, 2012
@TooTallNate TooTallNate readline: don't emit "line" events with a trailing '\n' char
Before this commit, readline was inconsistent in whether or not it would emit
"line" events with or without the trailing "\n" included. When "terminal"
mode was true, then there would be no "\n", when it was false, then the "\n"
would be present. However, the trailing "\n" doesn't add much, and most of the
time people just end up stripping it manually.

Part of #4243.
f3a3da4
@TooTallNate TooTallNate added a commit that referenced this issue Nov 7, 2012
@TooTallNate TooTallNate readline: don't emit "line" events with a trailing '\n' char
Before this commit, readline was inconsistent in whether or not it would emit
"line" events with or without the trailing "\n" included. When "terminal"
mode was true, then there would be no "\n", when it was false, then the "\n"
would be present. However, the trailing "\n" doesn't add much, and most of the
time people just end up stripping it manually.

Part of #4243.
e95e095
@TooTallNate

@Niggler @ackalker The fact that readline emits "line" events is by design. If you want to bypass the readline parser, simply write directly to the output stream instead of the readline instance.

But the bug regarding the trailing \n chars has now been fixed, so closing.

@TooTallNate TooTallNate closed this Nov 7, 2012
@drahgon55

@TooTallNate You miss the point @Niggler and @ackalker are talking about. The issue is not line events getting emitted by readline. It is the fact that ONLY the input stream should trigger line events. Writes to readline should not. It should work as a straight passthrough to the output stream write function when terminal:false.

Since this is still an issue a workaround is to write to the output stream directly. This can be done like so.

           var rl = readline.createInterface({
          input : fs.createReadStream(filePath,{encoding: "utf8"}),
          output: fs.createWriteStream(filePathOut, {flags: 'w',encoding: "utf8", mode: 0666 }),
          terminal: false
    })      
       rl.on('line',parseCB)
       function parseCB (line) {
              rl.output.write(line+'\n')  //<--- what you expect rl.write to do.
       }
@gibfahn gibfahn pushed a commit to ibmruntimes/node that referenced this issue Mar 29, 2016
@tomgco @jasnell tomgco + jasnell src: override v8 thread defaults using cli options
Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.

PR-URL: nodejs/node#4344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
03e07d3
@smanders smanders pushed a commit to smanders/node-v0.x-archive that referenced this issue Apr 8, 2016
@tomgco @evanlucas tomgco + evanlucas src: override v8 thread defaults using cli options
Based on the conversation in #4243 this implements a way to increase
and decrease the size of the thread pool used in v8.

Currently v8 restricts the thread pool size to `kMaxThreadPoolSize`
which at this commit is (4). So it is only possible to
decrease the thread pool size at the time of this commit. However with
changes upstream this could change at a later date.
If set to 0 then v8 would choose an appropriate size of the thread pool
based on the number of online processors.

PR-URL: nodejs/node#4344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
0d0c57f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment