From de5a5f5f7a01ae5400c9a246719c86dbea67da49 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Mon, 11 May 2015 14:00:07 -0700 Subject: [PATCH 1/9] repl: "fix" race condition between internal repl histories --- lib/internal/repl.js | 100 +++++++++++++++++------ src/node.js | 10 ++- test/parallel/test-internal-repl-race.js | 60 ++++++++++++++ 3 files changed, 142 insertions(+), 28 deletions(-) create mode 100644 test/parallel/test-internal-repl-race.js diff --git a/lib/internal/repl.js b/lib/internal/repl.js index 902e81f495573c..15de92c8f20f9c 100644 --- a/lib/internal/repl.js +++ b/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,7 +56,7 @@ function createRepl(env, cb) { } const repl = REPL.start(opts); - if (opts.terminal && env.NODE_REPL_HISTORY_FILE) { + if (env.NODE_REPL_HISTORY_FILE && attemptPersistentHistory) { return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, cb); } repl._historyPrev = _replHistoryMessage; @@ -64,27 +69,14 @@ function setupHistory(repl, historyPath, ready) { var writing = false; var pending = false; repl.pause(); - fs.open(historyPath, 'a+', oninit); + fs.readFile(historyPath, {encoding: 'utf8', flag: 'a+'}, ondata); - 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); - } - - 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 +90,16 @@ function setupHistory(repl, historyPath, ready) { } } - fs.open(historyPath, 'w', onhandle); + getTMPFile(ontmpfile); } - function onhandle(err, hnd) { + function ontmpfile(err, tmpinfo) { if (err) { return ready(err); } - repl._historyHandle = hnd; + repl._historyTMPInfo = tmpinfo; repl.on('line', online); - // reading the file data out erases it repl.once('flushHistory', function() { repl.resume(); ready(null, repl); @@ -134,11 +125,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); + writeAndSwapTMP(historyData, repl._historyTMPInfo, historyPath, onflushed); } - function onwritten(err, data) { + function onflushed(err, data) { writing = false; if (pending) { pending = false; @@ -165,3 +156,60 @@ function _replHistoryMessage() { this._historyPrev = Interface.prototype._historyPrev; return this._historyPrev(); } + + +function getTMPFile(ready) { + var tmpPath = os.tmpdir(); + if (!tmpPath) { + return ready(new Error('no tmpdir available')); + } + tmpPath = path.join(tmpPath, process.pid + '-' + 'repl.tmp'); + fs.open(tmpPath, 'w', 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, open a new tmpfile +function writeAndSwapTMP(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...'); + getTMPFile(ontmpfile); + } + + function ontmpfile(err, info) { + if (err) return ready(err); + tmpInfo.fd = info.fd; + tmpInfo.path = info.path; + debug('done!'); + return ready(null); + } +} diff --git a/src/node.js b/src/node.js index 55b1d782ce82b0..5ad3f3ca4249e6 100644 --- a/src/node.js +++ b/src/node.js @@ -131,9 +131,15 @@ 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.log('Encountered error with persistent history support.'); + 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) { diff --git a/test/parallel/test-internal-repl-race.js b/test/parallel/test-internal-repl-race.js new file mode 100644 index 00000000000000..dd31a9d413909b --- /dev/null +++ b/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); + }) + }) +} From ba80aa18c42b3207478438aa42e3139c1f96bd90 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Mon, 11 May 2015 14:02:26 -0700 Subject: [PATCH 2/9] style fix --- src/node.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node.js b/src/node.js index 5ad3f3ca4249e6..3ad42bfcbe38bf 100644 --- a/src/node.js +++ b/src/node.js @@ -134,12 +134,13 @@ cliRepl.createInternalRepl(process.env, true, function(err, repl) { if (err) { console.log('Encountered error with persistent history support.'); - return cliRepl.createInternalRepl(process.env, false, function (err, repl) { + 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) { From 46e2d6848ef33461757a4c43939c8b3adbf10784 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Mon, 11 May 2015 14:02:37 -0700 Subject: [PATCH 3/9] build: include internal modules in lint --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c87a8ea7f1cedf..cc20005baa4df3 100644 --- a/Makefile +++ b/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 From e4f0cfc988a358e70978238b7fca0715cebfeb68 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 11:37:46 -0700 Subject: [PATCH 4/9] @bnoordhuis: use "wx" for writing temp file --- lib/internal/repl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index 15de92c8f20f9c..ed40e28ce7fdc3 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -164,7 +164,7 @@ function getTMPFile(ready) { return ready(new Error('no tmpdir available')); } tmpPath = path.join(tmpPath, process.pid + '-' + 'repl.tmp'); - fs.open(tmpPath, 'w', 0o600, function(err, fd) { + fs.open(tmpPath, 'wx', 0o600, function(err, fd) { if (err) { return ready(err); } From 9b9dd70e7110be62e00eac57ea1e538221bd8044 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 11:44:18 -0700 Subject: [PATCH 5/9] add tmpfile cleanup on repl exit; replace TMP with Temp everywhere --- lib/internal/repl.js | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index ed40e28ce7fdc3..ab178d27107a6c 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -90,16 +90,16 @@ function setupHistory(repl, historyPath, ready) { } } - getTMPFile(ontmpfile); + getTempFile(ontmpfile); } function ontmpfile(err, tmpinfo) { if (err) { return ready(err); } - repl._historyTMPInfo = tmpinfo; + repl._historyTempInfo = tmpinfo; repl.on('line', online); - + repl.on('exit', removeTempFile); repl.once('flushHistory', function() { repl.resume(); ready(null, repl); @@ -126,7 +126,7 @@ function setupHistory(repl, historyPath, ready) { } writing = true; const historyData = JSON.stringify(repl.history || [], null, 2); - writeAndSwapTMP(historyData, repl._historyTMPInfo, historyPath, onflushed); + writeAndSwapTemp(historyData, repl._historyTempInfo, historyPath, onflushed); } function onflushed(err, data) { @@ -141,6 +141,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(); + }); + } } @@ -158,7 +169,7 @@ function _replHistoryMessage() { } -function getTMPFile(ready) { +function getTempFile(ready) { var tmpPath = os.tmpdir(); if (!tmpPath) { return ready(new Error('no tmpdir available')); @@ -177,7 +188,7 @@ function getTMPFile(ready) { // write data, sync the fd, close it, overwrite the target // with a rename, open a new tmpfile -function writeAndSwapTMP(data, tmpInfo, target, ready) { +function writeAndSwapTemp(data, tmpInfo, target, ready) { debug('writing tmp file... %s', tmpInfo.path, data); return fs.write(tmpInfo.fd, data, 0, 'utf8', onwritten); @@ -202,7 +213,7 @@ function writeAndSwapTMP(data, tmpInfo, target, ready) { function onrename(err) { if (err) return ready(err); debug('re-loading...'); - getTMPFile(ontmpfile); + getTempFile(ontmpfile); } function ontmpfile(err, info) { From 8752fe16631d07b87f667be41397df08789faf55 Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 11:45:43 -0700 Subject: [PATCH 6/9] @bnoordhuis: punctuate comments --- lib/internal/repl.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index ab178d27107a6c..972ff45e1da748 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -186,8 +186,8 @@ function getTempFile(ready) { }); } -// write data, sync the fd, close it, overwrite the target -// with a rename, open a new tmpfile +// Write data, sync the fd, close it, overwrite the target +// with a rename, and open a new tmpfile. function writeAndSwapTemp(data, tmpInfo, target, ready) { debug('writing tmp file... %s', tmpInfo.path, data); return fs.write(tmpInfo.fd, data, 0, 'utf8', onwritten); From bdf2b9f3a512f2f4147690cd4aa38041373ed45e Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 11:53:14 -0700 Subject: [PATCH 7/9] fix repl error state messaging and close on error --- lib/internal/repl.js | 7 ++++++- src/node.js | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index 972ff45e1da748..d6e9cbe68d9771 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -57,7 +57,12 @@ function createRepl(env, attemptPersistentHistory, cb) { const repl = REPL.start(opts); if (env.NODE_REPL_HISTORY_FILE && attemptPersistentHistory) { - return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, cb); + return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, function(err) { + if (err) { + repl.close(); + } + cb(err); + }); } repl._historyPrev = _replHistoryMessage; cb(null, repl); diff --git a/src/node.js b/src/node.js index 3ad42bfcbe38bf..258b51bacfe382 100644 --- a/src/node.js +++ b/src/node.js @@ -133,7 +133,11 @@ var cliRepl = Module.requireRepl(); cliRepl.createInternalRepl(process.env, true, function(err, repl) { if (err) { - console.log('Encountered error with persistent history support.'); + 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) { From 821332b83b8322707bf1601c1b42ad2c1975d38f Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 11:55:12 -0700 Subject: [PATCH 8/9] note why we use fs.write vs. writeFile --- lib/internal/repl.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index d6e9cbe68d9771..bee947bfd7cafa 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -193,6 +193,9 @@ function getTempFile(ready) { // 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); From 6158e2d3d8e06adba32d9a4ceb950cf6598e959d Mon Sep 17 00:00:00 2001 From: Chris Dickinson Date: Tue, 12 May 2015 12:16:11 -0700 Subject: [PATCH 9/9] @silverwind: add "node-" to repl name for clarity --- lib/internal/repl.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/repl.js b/lib/internal/repl.js index bee947bfd7cafa..97451821d74757 100644 --- a/lib/internal/repl.js +++ b/lib/internal/repl.js @@ -61,7 +61,7 @@ function createRepl(env, attemptPersistentHistory, cb) { if (err) { repl.close(); } - cb(err); + cb(err, repl); }); } repl._historyPrev = _replHistoryMessage; @@ -179,7 +179,7 @@ function getTempFile(ready) { if (!tmpPath) { return ready(new Error('no tmpdir available')); } - tmpPath = path.join(tmpPath, process.pid + '-' + 'repl.tmp'); + tmpPath = path.join(tmpPath, `${process.pid}-node-repl.tmp`); fs.open(tmpPath, 'wx', 0o600, function(err, fd) { if (err) { return ready(err);