Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: History file locking and other improvements #7005

Closed
wants to merge 7 commits into from
Closed

repl: History file locking and other improvements #7005

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30e2048
repl: History file locking and other improvements
lance May 25, 2016
be19adf
repl: Eliminate $NODE_HISTORY.LCK file.
lance Jun 1, 2016
ffc9cb2
repl: Recreate the history file with each write.
lance Jun 6, 2016
a74ca07
repl: Write history file in append mode on close.
lance Jun 21, 2016
f5302b4
repl: History file improvements
lance Jul 27, 2016
3741889
repl: Add a newline to end of the REPL history file
lance Jul 27, 2016
45d7504
repl: truncate history file before writing
lance Jul 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 19 additions & 58 deletions lib/internal/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -162,62 +160,25 @@ 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.writeSync(fd, history.join('\n'), 0, 'utf8');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ll want to append an extra newline, otherwise the first entry of the current session is added to the last entry of the previous one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax that's not actually the case. Since the history file is read in its entirety and rewritten each time, a trailing newline is not necessary. The pattern is:

  • On startup read the history file into memory and split on newline to create the in-memory history array.
  • Keep track of the number of lines found in the original history file.
  • On shutdown, extract the new lines added to the in-memory history.
  • Re-read the history file and concatenate the file data with the new lines from our in-memory history.
  • Write the entire history file with the data that was read from the file, plus the new lines that have been concatenated to it.
  • Close the history file.

No newline required! Also, note that the final fs.openSync used to write the history file is opened with the a flag, to get around the Windows issues with hidden files. When I do the subsequent fs.write, the write just begins at position 0 in the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, I ran 3 simultaneous REPLs and observed correct behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m going to try this again, but wouldn’t that mean that the a flag should not be used here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

> rm ~/.node_repl_history
$ ./node
> 1
1
> 2
2
> 3
3
> .exit
$ ./node
> 4
4
> 5
5
> 6
6
> .exit
$ cat ~/.node_repl_history
.exit
3
2
1.exit
6
5
4
.exit
3
2
1

Eek. Also, I personally consider it good practice to have all text files terminate with newlines. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on all points. I have been using ^D to exit the REPL in my tests
which must account for the difference. I will address this.

On Tue, Jul 26, 2016 at 6:14 PM Anna Henningsen [email protected]
wrote:

In lib/internal/repl.js
#7005 (comment):

 }
  • const fd = fs.openSync(historyPath, 'a');
  • fs.writeSync(fd, history.join('\n'), 0, 'utf8');

rm ~/.node_repl_history
$ ./node
11
22
33
.exit
$ ./node
44
55
66
.exit
$ cat ~/.node_repl_history.exit321.exit654.exit321

Eek. Also, I personally consider it good practice to have all text files
terminate with newlines. :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/7005/files/a74ca07852baafc5944c142d4c103c2067bb12cc#r72366519,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA-UKgrWsySmgECsaBZY2tlwkI1jV5Rks5qZrDzgaJpZM4In8hN
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax this is strange. I am not seeing this. I'll grant you that a newline terminating the file is good practice, and will address that. But for the record, I am not seeing the behavior you show above. I don't know why.....

~/s/node git:repl-history-file ❯❯❯ git status                                               ✭
On branch repl-history-file
nothing to commit, working directory clean
~/s/node git:repl-history-file ❯❯❯ git log -n1                                              ✭
commit f5302b44eeb48039d1e140b988e06b930fdeac5e
Author: Lance Ball <[email protected]>
Date:   Tue Jul 26 17:36:59 2016 -0700

    repl: History file improvements

    Address concerns in tests by using common.mustCall.
~/s/node git:repl-history-file ❯❯❯ rm ~/.node_repl_history                                  ✭
remove /Users/lanceball/.node_repl_history? y
~/s/node git:repl-history-file ❯❯❯ ./node                                                   ✭
> 1
1
> 2
2
> 3
3
> .exit
~/s/node git:repl-history-file ❯❯❯ ./node                                                   ✭
> 4
4
> 5
5
> 6
6
> .exit
~/s/node git:repl-history-file ❯❯❯ cat ~/.node_repl_history                                 ✭
.exit
6
5
4
.exit
3
2
1%

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird – I still can’t reproduce the behaviour it has for you… without much deeper digging, I’d assume that the a flag in this line is the reason for the doubling in my repl history file; is that wrong?

fs.closeSync(fd);
}
}


function _replHistoryMessage() {
if (this.history.length === 0) {
this._writeToOutput(
Expand Down
10 changes: 1 addition & 9 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
);
Expand Down
18 changes: 5 additions & 13 deletions test/parallel/test-repl-persistent-history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ const tests = [
env: {},
test: [UP, UP, ENTER],
expected: [prompt, prompt + '\'42\'', prompt + '\'=^.^=\'', '\'=^.^=\'\n',
prompt]
prompt]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you undo this whitespace change?

},
{
env: { NODE_REPL_HISTORY: historyPath,
Expand Down Expand Up @@ -189,7 +189,6 @@ const tests = [
];
const numtests = tests.length;


var testsNotRan = tests.length;

process.on('beforeExit', () =>
Expand All @@ -209,7 +208,7 @@ 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', runTest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have a common.mustCall()?


function runTest(assertCleaned) {
const opts = tests.shift();
Expand Down Expand Up @@ -247,7 +246,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();
Expand All @@ -262,16 +261,9 @@ function runTest(assertCleaned) {
throw err;
}

repl.once('close', () => {
if (repl._flushing) {
repl.once('flushHistory', onClose);
return;
}

onClose();
});
repl.once('exit', onExit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for common.mustCall?


function onClose() {
function onExit() {
const cleaned = after ? after() : cleanupTmpFile();

try {
Expand Down