Skip to content

Commit

Permalink
readline: use Date.now() and move test to parallel
Browse files Browse the repository at this point in the history
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.

Refs: nodejs#14674

PR-URL: nodejs#18563
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
apapirovski authored and BridgeAR committed Feb 7, 2018
1 parent 5ecd2e1 commit af5632e
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 114 deletions.
10 changes: 4 additions & 6 deletions lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ const {
kClearScreenDown
} = CSI;

const { now } = process.binding('timer_wrap').Timer;

const kHistorySize = 30;
const kMincrlfDelay = 100;
// \r\n, \n, or \r followed by something other than \n
Expand Down Expand Up @@ -411,7 +409,7 @@ Interface.prototype._normalWrite = function(b) {
}
var string = this._decoder.write(b);
if (this._sawReturnAt &&
now() - this._sawReturnAt <= this.crlfDelay) {
Date.now() - this._sawReturnAt <= this.crlfDelay) {
string = string.replace(/^\n/, '');
this._sawReturnAt = 0;
}
Expand All @@ -424,7 +422,7 @@ Interface.prototype._normalWrite = function(b) {
this._line_buffer = null;
}
if (newPartContainsEnding) {
this._sawReturnAt = string.endsWith('\r') ? now() : 0;
this._sawReturnAt = string.endsWith('\r') ? Date.now() : 0;

// got one or more newlines; process into "line" events
var lines = string.split(lineEnding);
Expand Down Expand Up @@ -917,14 +915,14 @@ Interface.prototype._ttyWrite = function(s, key) {

switch (key.name) {
case 'return': // carriage return, i.e. \r
this._sawReturnAt = now();
this._sawReturnAt = Date.now();
this._line();
break;

case 'enter':
// When key interval > crlfDelay
if (this._sawReturnAt === 0 ||
now() - this._sawReturnAt > this.crlfDelay) {
Date.now() - this._sawReturnAt > this.crlfDelay) {
this._line();
}
this._sawReturnAt = 0;
Expand Down
77 changes: 77 additions & 0 deletions test/parallel/test-readline-interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -869,3 +869,80 @@ function isWarned(emitter) {
assert.strictEqual(rl._prompt, '$ ');
}
});

// For the purposes of the following tests, we do not care about the exact
// value of crlfDelay, only that the behaviour conforms to what's expected.
// Setting it to Infinity allows the test to succeed even under extreme
// CPU stress.
const crlfDelay = Infinity;

[ true, false ].forEach(function(terminal) {
// sending multiple newlines at once that does not end with a new line
// and a `end` event(last line is)

// \r\n should emit one line event, not two
{
const fi = new FakeInput();
const rli = new readline.Interface(
{
input: fi,
output: fi,
terminal: terminal,
crlfDelay
}
);
const expectedLines = ['foo', 'bar', 'baz', 'bat'];
let callCount = 0;
rli.on('line', function(line) {
assert.strictEqual(line, expectedLines[callCount]);
callCount++;
});
fi.emit('data', expectedLines.join('\r\n'));
assert.strictEqual(callCount, expectedLines.length - 1);
rli.close();
}

// \r\n should emit one line event when split across multiple writes.
{
const fi = new FakeInput();
const rli = new readline.Interface({
input: fi,
output: fi,
terminal: terminal,
crlfDelay
});
const expectedLines = ['foo', 'bar', 'baz', 'bat'];
let callCount = 0;
rli.on('line', function(line) {
assert.strictEqual(line, expectedLines[callCount]);
callCount++;
});
expectedLines.forEach(function(line) {
fi.emit('data', `${line}\r`);
fi.emit('data', '\n');
});
assert.strictEqual(callCount, expectedLines.length);
rli.close();
}

// Emit one line event when the delay between \r and \n is
// over the default crlfDelay but within the setting value.
{
const fi = new FakeInput();
const delay = 125;
const rli = new readline.Interface({
input: fi,
output: fi,
terminal: terminal,
crlfDelay
});
let callCount = 0;
rli.on('line', () => callCount++);
fi.emit('data', '\r');
setTimeout(common.mustCall(() => {
fi.emit('data', '\n');
assert.strictEqual(callCount, 1);
rli.close();
}), delay);
}
});
108 changes: 0 additions & 108 deletions test/sequential/test-readline-interface.js

This file was deleted.

0 comments on commit af5632e

Please sign in to comment.