"Fix" repl race condition

Makefile

@@ -387,7 +387,7 @@ bench-idle:
 	./$(NODE_EXE) benchmark/idle_clients.js &
 
 jslint:
-	./$(NODE_EXE) tools/eslint/bin/eslint.js src/*.js lib/*.js --reset --quiet
+	./$(NODE_EXE) tools/eslint/bin/eslint.js src/*.js lib/*.js lib/internal/*.js --reset --quiet
 
 CPPLINT_EXCLUDE ?=
 CPPLINT_EXCLUDE += src/node_lttng.cc

lib/internal/repl.js

@@ -3,6 +3,9 @@
 const Interface = require('readline').Interface;
 const REPL = require('repl');
 const path = require('path');
+const util = require('util');
+
+const debug = util.debuglog('repl');
 
 module.exports = Object.create(REPL);
 module.exports.createInternalRepl = createRepl;
@@ -17,16 +20,18 @@ function replStart() {
   return REPL.start.apply(REPL, arguments);
 }
 
-function createRepl(env, cb) {
+function createRepl(env, attemptPersistentHistory, cb) {
   const opts = {
     ignoreUndefined: false,
-    terminal: process.stdout.isTTY,
     useGlobal: true
   };
 
   if (parseInt(env.NODE_NO_READLINE)) {
     opts.terminal = false;
   }
+  if (parseInt(env.NODE_FORCE_READLINE)) {
+    opts.terminal = true;
+  }
   if (parseInt(env.NODE_DISABLE_COLORS)) {
     opts.useColors = false;
   }
@@ -51,8 +56,13 @@ function createRepl(env, cb) {
   }
 
   const repl = REPL.start(opts);
-  if (opts.terminal && env.NODE_REPL_HISTORY_FILE) {
-    return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, cb);
+  if (env.NODE_REPL_HISTORY_FILE && attemptPersistentHistory) {
+    return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, function(err) {
+      if (err) {
+        repl.close();
+      }
+      cb(err, repl);
+    });
   }
   repl._historyPrev = _replHistoryMessage;
   cb(null, repl);
@@ -64,27 +74,14 @@ function setupHistory(repl, historyPath, ready) {
   var writing = false;
   var pending = false;
   repl.pause();
-  fs.open(historyPath, 'a+', oninit);
-
-  function oninit(err, hnd) {
-    if (err) {
-      return ready(err);
-    }
-    fs.close(hnd, onclose);
-  }
-
-  function onclose(err) {
-    if (err) {
-      return ready(err);
-    }
-    fs.readFile(historyPath, 'utf8', onread);
-  }
+  fs.readFile(historyPath, {encoding: 'utf8', flag: 'a+'}, ondata);
 
-  function onread(err, data) {
+  function ondata(err, data) {
     if (err) {
       return ready(err);
     }
 
+    debug('loaded %s', historyPath);
     if (data) {
       try {
         repl.history = JSON.parse(data);
@@ -98,17 +95,16 @@ function setupHistory(repl, historyPath, ready) {
       }
     }
 
-    fs.open(historyPath, 'w', onhandle);
+    getTempFile(ontmpfile);
   }
 
-  function onhandle(err, hnd) {
+  function ontmpfile(err, tmpinfo) {
     if (err) {
       return ready(err);
     }
-    repl._historyHandle = hnd;
+    repl._historyTempInfo = tmpinfo;
     repl.on('line', online);
-
-    // reading the file data out erases it
+    repl.on('exit', removeTempFile);
     repl.once('flushHistory', function() {
       repl.resume();
       ready(null, repl);
@@ -134,11 +130,11 @@ function setupHistory(repl, historyPath, ready) {
       return;
     }
     writing = true;
-    const historyData = JSON.stringify(repl.history, null, 2);
-    fs.write(repl._historyHandle, historyData, 0, 'utf8', onwritten);
+    const historyData = JSON.stringify(repl.history || [], null, 2);
+    writeAndSwapTemp(historyData, repl._historyTempInfo, historyPath, onflushed);
   }
 
-  function onwritten(err, data) {
+  function onflushed(err, data) {
     writing = false;
     if (pending) {
       pending = false;
@@ -150,6 +146,17 @@ function setupHistory(repl, historyPath, ready) {
       }
     }
   }
+
+  function removeTempFile() {
+    if (repl._flushing)
+      return repl.once('flushHistory', function() {
+        removeTempFile();
+      });
+    repl._flushing = true;
+    fs.unlink(repl._historyTempInfo.path, function() {
+      onflushed();
+    });
+  }
 }
 
 
@@ -165,3 +172,63 @@ function _replHistoryMessage() {
   this._historyPrev = Interface.prototype._historyPrev;
   return this._historyPrev();
 }
+
+
+function getTempFile(ready) {
+  var tmpPath = os.tmpdir();
+  if (!tmpPath) {
+    return ready(new Error('no tmpdir available'));
+  }
+  tmpPath = path.join(tmpPath, `${process.pid}-node-repl.tmp`);
+  fs.open(tmpPath, 'wx', 0o600, function(err, fd) {
+    if (err) {
+      return ready(err);
+    }
+    return ready(null, {
+      fd: fd,
+      path: tmpPath
+    });
+  });
+}
+
+// Write data, sync the fd, close it, overwrite the target
+// with a rename, and open a new tmpfile.
+//
+// We use fs.write instead of writeFile because the latter
+// does not accept an fd at the time of writing.
+function writeAndSwapTemp(data, tmpInfo, target, ready) {
+  debug('writing tmp file... %s', tmpInfo.path, data);
+  return fs.write(tmpInfo.fd, data, 0, 'utf8', onwritten);
+
+  function onwritten(err) {
+    if (err) return ready(err);
+    debug('syncing... %s', tmpInfo.path);
+    fs.fsync(tmpInfo.fd, onsync);
+  }
+
+  function onsync(err) {
+    if (err) return ready(err);
+    debug('closing... %s', tmpInfo.path);
+    fs.close(tmpInfo.fd, onclose);
+  }
+
+  function onclose(err) {
+    if (err) return ready(err);
+    debug('rename... %s -> %s', tmpInfo.path, target);
+    fs.rename(tmpInfo.path, target, onrename);
+  }
+
+  function onrename(err) {
+    if (err) return ready(err);
+    debug('re-loading...');
+    getTempFile(ontmpfile);
+  }
+
+  function ontmpfile(err, info) {
+    if (err) return ready(err);
+    tmpInfo.fd = info.fd;
+    tmpInfo.path = info.path;
+    debug('done!');
+    return ready(null);
+  }
+}

src/node.js

@@ -131,9 +131,20 @@
         if (process._forceRepl || NativeModule.require('tty').isatty(0)) {
           // REPL
           var cliRepl = Module.requireRepl();
-          cliRepl.createInternalRepl(process.env, function(err, repl) {
+          cliRepl.createInternalRepl(process.env, true, function(err, repl) {
             if (err) {
-              throw err;
+              console.error('Encountered error with persistent history support.')
+              console.error('Run with NODE_DEBUG=repl for more information.');
+              if (/repl/.test(process.env.NODE_DEBUG || '')) {
+                console.error(err.stack);
+              }
+              return cliRepl.createInternalRepl(
+                process.env, false, function(err, repl) {
+                if (err) {
+                  throw err;
+                }
+                repl.on('exit', process.exit);
+              });
             }
             repl.on('exit', function() {
               if (repl._flushing) {

test/parallel/test-internal-repl-race.js

@@ -0,0 +1,60 @@
+var spawn = require('child_process').spawn;
+var common = require('../common');
+var assert = require('assert');
+var path = require('path');
+var os = require('os');
+var fs = require('fs');
+
+var filename = path.join(common.tmpDir, 'node-history.json');
+
+var config = {
+  env: {
+    NODE_REPL_HISTORY_FILE: filename,
+    NODE_FORCE_READLINE: 1
+  }
+}
+
+var lhs = spawn(process.execPath, ['-i'], config);
+var rhs = spawn(process.execPath, ['-i'], config);
+
+lhs.stdout.pipe(process.stdout);
+rhs.stdout.pipe(process.stdout);
+lhs.stderr.pipe(process.stderr);
+rhs.stderr.pipe(process.stderr);
+
+
+var i = 0;
+var tried = 0;
+var exitcount = 0;
+while (i++ < 100) {
+  lhs.stdin.write('hello = 0' + os.EOL);
+  rhs.stdin.write('OK = 0' + os.EOL);
+}
+lhs.stdin.write('\u0004')
+rhs.stdin.write('\u0004')
+
+lhs.once('exit', onexit)
+rhs.once('exit', onexit)
+
+function onexit() {
+  if (++exitcount !== 2) {
+    return;
+  }
+  check();
+}
+
+function check() {
+  fs.readFile(filename, 'utf8', function(err, data) {
+    if (err) {
+      console.log(err)
+      if (++tried > 100) {
+        throw err;
+      }
+      return setTimeout(check, 15);
+    }
+    assert.doesNotThrow(function() {
+      console.log(data)
+      JSON.parse(data);
+    })
+  })
+}