repl: History file locking and other improvements

lib/internal/repl.js

@@ -10,10 +10,6 @@ const debug = require('util').debuglog('repl');
 module.exports = Object.create(REPL);
 module.exports.createInternalRepl = createRepl;
 
-// XXX(chrisdickinson): The 15ms debounce value is somewhat arbitrary.
-// The debounce is to guard against code pasted into the REPL.
-const kDebounceHistoryMS = 15;
-
 function createRepl(env, opts, cb) {
   if (typeof opts === 'function') {
     cb = opts;
@@ -65,8 +61,8 @@ function createRepl(env, opts, cb) {
 }
 
 function setupHistory(repl, historyPath, oldHistoryPath, ready) {
+  let initialHistoryLength = 0;
   // Empty string disables persistent history.
-
   if (typeof historyPath === 'string')
     historyPath = historyPath.trim();
 
@@ -89,9 +85,6 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     }
   }
 
-  var timer = null;
-  var writing = false;
-  var pending = false;
   repl.pause();
   // History files are conventionally not readable by others:
   // https://github.com/nodejs/node/issues/3392
@@ -121,13 +114,18 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     fs.readFile(historyPath, 'utf8', onread);
   }
 
+  function parseData(data) {
+    return data.toString().trim().split(/[\n\r]+/).slice(0, repl.historySize);
+  }
+
   function onread(err, data) {
     if (err) {
       return ready(err);
     }
 
     if (data) {
-      repl.history = data.split(/[\n\r]+/, repl.historySize);
+      repl.history = parseData(data);
+      initialHistoryLength = repl.history.length;
     } else if (oldHistoryPath === historyPath) {
       // If pre-v3.0, the user had set NODE_REPL_HISTORY_FILE to
       // ~/.node_repl_history, warn the user about it and proceed.
@@ -162,62 +160,26 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
         }
       }
     }
-
-    fs.open(historyPath, 'w', onhandle);
-  }
-
-  function onhandle(err, hnd) {
-    if (err) {
-      return ready(err);
-    }
-    repl._historyHandle = hnd;
-    repl.on('line', online);
-
-    // reading the file data out erases it
-    repl.once('flushHistory', function() {
-      repl.resume();
-      ready(null, repl);
-    });
-    flushHistory();
-  }
-
-  // ------ history listeners ------
-  function online() {
-    repl._flushing = true;
-
-    if (timer) {
-      clearTimeout(timer);
-    }
-
-    timer = setTimeout(flushHistory, kDebounceHistoryMS);
+    // Flush history on close
+    repl.on('close', flushHistory);
+    repl.resume();
+    ready(null, repl);
   }
 
   function flushHistory() {
-    timer = null;
-    if (writing) {
-      pending = true;
-      return;
-    }
-    writing = true;
-    const historyData = repl.history.join(os.EOL);
-    fs.write(repl._historyHandle, historyData, 0, 'utf8', onwritten);
-  }
-
-  function onwritten(err, data) {
-    writing = false;
-    if (pending) {
-      pending = false;
-      online();
-    } else {
-      repl._flushing = Boolean(timer);
-      if (!repl._flushing) {
-        repl.emit('flushHistory');
-      }
+    const data = fs.readFileSync(historyPath, 'utf8');
+    const newLines = repl.history.length - initialHistoryLength;
+    let history = repl.history.slice(0, newLines);
+    if (data) {
+      history = history.concat(parseData(data)).slice(0, repl.historySize);
     }
+    const fd = fs.openSync(historyPath, 'a');
+    fs.ftruncateSync(fd, 0);
+    fs.writeSync(fd, history.join('\n') + '\n', 0, 'utf8');
+    fs.closeSync(fd);
   }
 }
 
-
 function _replHistoryMessage() {
   if (this.history.length === 0) {
     this._writeToOutput(

lib/repl.js

@@ -370,7 +370,7 @@ function REPLServer(prompt,
   self.setPrompt(self._prompt);
 
   self.on('close', function() {
-    self.emit('exit');
+    process.nextTick(() => self.emit('exit'));
   });
 
   var sawSIGINT = false;
@@ -525,14 +525,6 @@ exports.start = function(prompt,
 };
 
 REPLServer.prototype.close = function replClose() {
-  if (this.terminal && this._flushing && !this._closingOnFlush) {
-    this._closingOnFlush = true;
-    this.once('flushHistory', () =>
-      Interface.prototype.close.call(this)
-    );
-
-    return;
-  }
   process.nextTick(() =>
     Interface.prototype.close.call(this)
   );

test/parallel/test-repl-persistent-history.js

@@ -189,7 +189,6 @@ const tests = [
 ];
 const numtests = tests.length;
 
-
 var testsNotRan = tests.length;
 
 process.on('beforeExit', () =>
@@ -209,7 +208,8 @@ function cleanupTmpFile() {
 
 // Copy our fixture to the tmp directory
 fs.createReadStream(historyFixturePath)
-  .pipe(fs.createWriteStream(historyPath)).on('unpipe', () => runTest());
+  .pipe(fs.createWriteStream(historyPath)).on('unpipe',
+  common.mustCall(runTest));
 
 function runTest(assertCleaned) {
   const opts = tests.shift();
@@ -247,7 +247,7 @@ function runTest(assertCleaned) {
         try {
           assert.strictEqual(output, expected.shift());
         } catch (err) {
-          console.error(`Failed test # ${numtests - tests.length}`);
+          console.error(`Failed test # ${numtests - tests.length}. ${err}`);
           throw err;
         }
         next();
@@ -262,16 +262,9 @@ function runTest(assertCleaned) {
       throw err;
     }
 
-    repl.once('close', () => {
-      if (repl._flushing) {
-        repl.once('flushHistory', onClose);
-        return;
-      }
-
-      onClose();
-    });
+    repl.once('exit', common.mustCall(onExit));
 
-    function onClose() {
+    function onExit() {
       const cleaned = after ? after() : cleanupTmpFile();
 
       try {