Skip to content

Commit eda075e

Browse files
committed
readline: use Date.now() and move test to parallel
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
1 parent 47a984a commit eda075e

File tree

3 files changed

+80
-114
lines changed

3 files changed

+80
-114
lines changed

lib/readline.js

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,6 @@ const {
4848
kClearScreenDown
4949
} = CSI;
5050

51-
const { now } = process.binding('timer_wrap').Timer;
52-
5351
const kHistorySize = 30;
5452
const kMincrlfDelay = 100;
5553
// \r\n, \n, or \r followed by something other than \n
@@ -409,9 +407,10 @@ Interface.prototype._normalWrite = function(b) {
409407
if (b === undefined) {
410408
return;
411409
}
410+
const now = Date.now();
412411
var string = this._decoder.write(b);
413412
if (this._sawReturnAt &&
414-
now() - this._sawReturnAt <= this.crlfDelay) {
413+
now - this._sawReturnAt <= this.crlfDelay) {
415414
string = string.replace(/^\n/, '');
416415
this._sawReturnAt = 0;
417416
}
@@ -424,7 +423,7 @@ Interface.prototype._normalWrite = function(b) {
424423
this._line_buffer = null;
425424
}
426425
if (newPartContainsEnding) {
427-
this._sawReturnAt = string.endsWith('\r') ? now() : 0;
426+
this._sawReturnAt = string.endsWith('\r') ? now : 0;
428427

429428
// got one or more newlines; process into "line" events
430429
var lines = string.split(lineEnding);
@@ -911,20 +910,22 @@ Interface.prototype._ttyWrite = function(s, key) {
911910
} else {
912911
/* No modifier keys used */
913912

913+
const now = Date.now();
914+
914915
// \r bookkeeping is only relevant if a \n comes right after.
915916
if (this._sawReturnAt && key.name !== 'enter')
916917
this._sawReturnAt = 0;
917918

918919
switch (key.name) {
919920
case 'return': // carriage return, i.e. \r
920-
this._sawReturnAt = now();
921+
this._sawReturnAt = now;
921922
this._line();
922923
break;
923924

924925
case 'enter':
925926
// When key interval > crlfDelay
926927
if (this._sawReturnAt === 0 ||
927-
now() - this._sawReturnAt > this.crlfDelay) {
928+
now - this._sawReturnAt > this.crlfDelay) {
928929
this._line();
929930
}
930931
this._sawReturnAt = 0;

test/parallel/test-readline-interface.js

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ class FakeInput extends EventEmitter {
3636
end() {}
3737
}
3838

39+
const crlfDelay = common.platformTimeout(1000);
40+
3941
function isWarned(emitter) {
4042
for (const name in emitter) {
4143
const listeners = emitter[name];
@@ -869,3 +871,74 @@ function isWarned(emitter) {
869871
assert.strictEqual(rl._prompt, '$ ');
870872
}
871873
});
874+
875+
[ true, false ].forEach(function(terminal) {
876+
// sending multiple newlines at once that does not end with a new line
877+
// and a `end` event(last line is)
878+
879+
// \r\n should emit one line event, not two
880+
{
881+
const fi = new FakeInput();
882+
const rli = new readline.Interface(
883+
{
884+
input: fi,
885+
output: fi,
886+
terminal: terminal,
887+
crlfDelay
888+
}
889+
);
890+
const expectedLines = ['foo', 'bar', 'baz', 'bat'];
891+
let callCount = 0;
892+
rli.on('line', function(line) {
893+
assert.strictEqual(line, expectedLines[callCount]);
894+
callCount++;
895+
});
896+
fi.emit('data', expectedLines.join('\r\n'));
897+
assert.strictEqual(callCount, expectedLines.length - 1);
898+
rli.close();
899+
}
900+
901+
// \r\n should emit one line event when split across multiple writes.
902+
{
903+
const fi = new FakeInput();
904+
const rli = new readline.Interface({
905+
input: fi,
906+
output: fi,
907+
terminal: terminal,
908+
crlfDelay
909+
});
910+
const expectedLines = ['foo', 'bar', 'baz', 'bat'];
911+
let callCount = 0;
912+
rli.on('line', function(line) {
913+
assert.strictEqual(line, expectedLines[callCount]);
914+
callCount++;
915+
});
916+
expectedLines.forEach(function(line) {
917+
fi.emit('data', `${line}\r`);
918+
fi.emit('data', '\n');
919+
});
920+
assert.strictEqual(callCount, expectedLines.length);
921+
rli.close();
922+
}
923+
924+
// Emit one line event when the delay between \r and \n is
925+
// over the default crlfDelay but within the setting value.
926+
{
927+
const fi = new FakeInput();
928+
const delay = 125;
929+
const rli = new readline.Interface({
930+
input: fi,
931+
output: fi,
932+
terminal: terminal,
933+
crlfDelay
934+
});
935+
let callCount = 0;
936+
rli.on('line', () => callCount++);
937+
fi.emit('data', '\r');
938+
setTimeout(common.mustCall(() => {
939+
fi.emit('data', '\n');
940+
assert.strictEqual(callCount, 1);
941+
rli.close();
942+
}), delay);
943+
}
944+
});

test/sequential/test-readline-interface.js

Lines changed: 0 additions & 108 deletions
This file was deleted.

0 commit comments

Comments
 (0)