From 9f1370fb4f8084acb69e4361bf2dca3b0b796ddb Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 7 Aug 2017 17:11:06 -0700 Subject: [PATCH 1/2] lib: use Timer.now() in readline module Using Date.now() introduces problems when operating under load or otherwise with constrained resources. Use Timer.now() to mitigate. The problem was identified in `test-readline-interface` where under heavy load, `\r` and `\n` were received so far apart that they were treated as separate line endings rather than a single line ending. Switching to `Timer.now()` prevented this from happening. Refs: https://github.com/nodejs/node/issues/14674 --- lib/readline.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/readline.js b/lib/readline.js index 57f9e7d6e8c849..5e96a04b1c5e98 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -48,6 +48,8 @@ const { kClearScreenDown } = CSI; +const now = process.binding('timer_wrap').Timer.now; + const kHistorySize = 30; const kMincrlfDelay = 100; // \r\n, \n, or \r followed by something other than \n @@ -409,7 +411,7 @@ Interface.prototype._normalWrite = function(b) { } var string = this._decoder.write(b); if (this._sawReturnAt && - Date.now() - this._sawReturnAt <= this.crlfDelay) { + now() - this._sawReturnAt <= this.crlfDelay) { string = string.replace(/^\n/, ''); this._sawReturnAt = 0; } @@ -422,7 +424,7 @@ Interface.prototype._normalWrite = function(b) { this._line_buffer = null; } if (newPartContainsEnding) { - this._sawReturnAt = string.endsWith('\r') ? Date.now() : 0; + this._sawReturnAt = string.endsWith('\r') ? now() : 0; // got one or more newlines; process into "line" events var lines = string.split(lineEnding); @@ -916,14 +918,14 @@ Interface.prototype._ttyWrite = function(s, key) { switch (key.name) { case 'return': // carriage return, i.e. \r - this._sawReturnAt = Date.now(); + this._sawReturnAt = now(); this._line(); break; case 'enter': // When key interval > crlfDelay if (this._sawReturnAt === 0 || - Date.now() - this._sawReturnAt > this.crlfDelay) { + now() - this._sawReturnAt > this.crlfDelay) { this._line(); } this._sawReturnAt = 0; From 710304c89d65cd5728eed6e6402040851fa5295c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 7 Aug 2017 17:17:31 -0700 Subject: [PATCH 2/2] test: split out load-sensitive readline tests Two test cases in `test-readline-interface` are sensitive to resource constraints (probably due to `\r` and `\n` not arriving within the appropriate delay to be treated as a single line ending). Move those tests to `sequential`. Fixes: https://github.com/nodejs/node/issues/14674 --- test/parallel/test-readline-interface.js | 38 +--------- test/sequential/test-readline-interface.js | 86 ++++++++++++++++++++++ 2 files changed, 87 insertions(+), 37 deletions(-) create mode 100644 test/sequential/test-readline-interface.js diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index dd96e17362efcc..758a8d3d926931 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -22,6 +22,7 @@ // Flags: --expose_internals 'use strict'; const common = require('../common'); + const assert = require('assert'); const readline = require('readline'); const internalReadline = require('internal/readline'); @@ -233,43 +234,6 @@ function isWarned(emitter) { // 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 } - ); - 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 } - ); - 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(); - } - // \r should behave like \n when alone { const fi = new FakeInput(); diff --git a/test/sequential/test-readline-interface.js b/test/sequential/test-readline-interface.js new file mode 100644 index 00000000000000..915bcdd0c0750c --- /dev/null +++ b/test/sequential/test-readline-interface.js @@ -0,0 +1,86 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +// Flags: --expose_internals +'use strict'; +require('../common'); + +// These test cases are in `sequential` rather than the analogous test file in +// `parallel` because they become unrelaible under load. The unreliability under +// load was determined empirically when the test cases were in `parallel` by +// running: +// tools/test.py -j 96 --repeat 192 test/parallel/test-readline-interface.js + +const assert = require('assert'); +const readline = require('readline'); +const EventEmitter = require('events').EventEmitter; +const inherits = require('util').inherits; + +function FakeInput() { + EventEmitter.call(this); +} +inherits(FakeInput, EventEmitter); +FakeInput.prototype.resume = () => {}; +FakeInput.prototype.pause = () => {}; +FakeInput.prototype.write = () => {}; +FakeInput.prototype.end = () => {}; + +[ 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 } + ); + 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 } + ); + 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(); + } +});