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 1 commit
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
64 changes: 39 additions & 25 deletions lib/internal/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
}

if (data) {
repl.history = data.split(/[\n\r]+/, repl.historySize);
repl.history = data.trim().split(/[\n\r]+/)
.reverse().slice(0, repl.historySize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this PR change the ordering in the history file? if so, it is probably semver-major.

Copy link
Member

Choose a reason for hiding this comment

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

Umm… I think the intent of repl.historySize is to limit the file size, not the length of the history that’s being used?

Copy link
Contributor

@Fishrock123 Fishrock123 May 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's provided by readline to limit the history in memory. But we use it for both.

Copy link
Member

Choose a reason for hiding this comment

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

@Fishrock123 Thanks for the clarification… It still seems to me (both from looking at the code and from experimenting) the file grows without bounds with this PR in its current state.

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 @Fishrock123 historically, historySize has limited both the file size, and the maximum number of lines loaded into memory at start up. But this almost seems like a side effect of readline usage, and is not documented anywhere that I could find in the context of the REPL.

This PR changes it so that so that the number of lines stored in memory from the history file at start up is still limited, but the history file size itself is unbounded.

Copy link
Member

Choose a reason for hiding this comment

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

This PR changes it so that so that the number of lines stored in memory from the history file at start up is still limited, but the history file size itself is unbounded.

Is there any particular advantage to limiting the number of lines stored in memory if they are all going to be loaded (and saved?) anyway?

} 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 @@ -163,7 +164,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
}
}

fs.open(historyPath, 'w', onhandle);
fs.open(historyPath, 'a', onhandle);
}

function onhandle(err, hnd) {
Expand All @@ -172,13 +173,8 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
}
repl._historyHandle = hnd;
repl.on('line', online);

// reading the file data out erases it
repl.once('flushHistory', function() {
repl.resume();
ready(null, repl);
});
flushHistory();
repl.resume();
ready(null, repl);
}

// ------ history listeners ------
Expand All @@ -192,31 +188,49 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
timer = setTimeout(flushHistory, kDebounceHistoryMS);
}

const historyLockFile = `${historyPath}.LCK`;
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');
// Ref: https://github.com/nodejs/node/issues/1634
// opening a file for exclusive write should be atomic
// on non-networked POSIX systems.
fs.open(historyLockFile, 'wx', (err, fd) => {
if (err && err.code === 'EEXIST') {
pending = true;
repl.resume();
return;
}
}
writing = true;
const lastLine = repl.history[0] + '\n';
fs.write(repl._historyHandle, lastLine, NaN, 'utf8', onwritten(fd));
});
}
}

function onwritten(fd) {
return (err, data) => {
writing = false;
if (pending) {
pending = false;
online();
} else {
repl._flushing = Boolean(timer);
if (!repl._flushing) {
repl.emit('flushHistory');
}
}
unlockHistoryFile(fd);
Copy link
Member

@addaleax addaleax May 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Figured out where my .exit problem comes from – You need to call unlockHistoryFile before flushHistory is emitted, because the latter one is calling process.exit() in lib/internal/bootstrap_node.js.

(EDIT: Which may also be a good reason to do everything inside of unlockHistoryFile synchronously.)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - I will add a fix and a test for this.

Copy link
Member Author

@lance lance May 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm - I can only reproduce this when there is already a stray .node_repl_history.LCK file at start up, and the history file is never written to. When the REPLServer is closing it adds a listener for flushHistory here which never gets emitted, and so the final Interface#close is never called.

I can add a check for the existence of the .LCK file at startup and then remove it if necessary. There are maybe a couple of ways this could happen.

  • Notify the user. "Hey! There's a lock file, do you want me to delete it?". Probably the safest, but pretty intrusive and kind of looks like its underwear is showing.
  • Set one or more brief timeouts to check/recheck for the existence before deleting. This doesn't eliminate the possibility of a race condition, but it definitely minimizes it.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm - I can only reproduce this when there is already a stray .node_repl_history.LCK file at start up, and the history file is never written to.

Well, yeah, this is how I can reproduce it:

$ ./node
> .exit
$ ./node
> .exit
[hangs]

The first one here leaves the stray lock file lying around.

This is just an idea, but how about this approach when saving the history:

  1. Read the current history file into memory
  2. Remove it
  3. Create the history file again, with flags set to wx. If that fails, go back to 1.
  4. Write the desired number of lines to the opened history file

That seems to be free of race conditions and does not require a lock file that could keep lying around if the process crashes (for whatever reason, it doesn’t have to be REPL-related)… what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

And btw, this is how bash/readline does it:

  • No locking whatsoever
  • On exit, append the current session’s history to .bash_history
  • After that, read the entire history file, truncate in memory to the desired number of lines, and then save again

This approach obviously racy but apparently that’s sufficiently irrelevant to just not care.

Copy link
Member Author

@lance lance Jun 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax I've been playing with an implementation of your algorithm. It works, but I'm concerned about removing/recreating the history file. We would want to recreate it with the same file modes as the original. If I use fs.stat(), can I use the resulting stats.mode in fs.open() when recreating the file? I don't think that would work, would it?

Edit: It does work on OSX. Not sure about Windows - specifically the hidden bit.

Copy link
Member

Choose a reason for hiding this comment

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

@lance Valid point, and I don’t know if it works either (but I’d actually guess that it does)… if you have the code for that lying around somewhere, I can try and test it manually.

};
}

function unlockHistoryFile(fd) {
fs.close(fd);
fs.unlink(historyLockFile);
}
}

function _replHistoryMessage() {
if (this.history.length === 0) {
Expand Down
11 changes: 5 additions & 6 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 @@ -115,19 +115,19 @@ const tests = [
{
env: { NODE_REPL_HISTORY: historyPath },
test: [UP, CLEAR],
expected: [prompt, prompt + '\'you look fabulous today\'', prompt]
expected: [prompt, prompt + '\'Stay Fresh~\'', prompt]
},
{
env: { NODE_REPL_HISTORY: historyPath,
NODE_REPL_HISTORY_FILE: oldHistoryPath },
test: [UP, CLEAR],
expected: [prompt, prompt + '\'you look fabulous today\'', prompt]
expected: [prompt, prompt + '\'Stay Fresh~\'', prompt]
},
{
env: { NODE_REPL_HISTORY: historyPath,
NODE_REPL_HISTORY_FILE: '' },
test: [UP, CLEAR],
expected: [prompt, prompt + '\'you look fabulous today\'', prompt]
expected: [prompt, prompt + '\'Stay Fresh~\'', prompt]
},
{
env: {},
Expand All @@ -154,14 +154,13 @@ const tests = [
{ // Requires the above testcase
env: {},
test: [UP, UP, ENTER],
expected: [prompt, prompt + '\'42\'', prompt + '\'=^.^=\'', '\'=^.^=\'\n',
prompt]
expected: [prompt, prompt + '\'42\'', '\'42\'\n', prompt]
},
{
env: { NODE_REPL_HISTORY: historyPath,
NODE_REPL_HISTORY_SIZE: 1 },
test: [UP, UP, CLEAR],
expected: [prompt, prompt + '\'you look fabulous today\'', prompt]
expected: [prompt, prompt + '\'Stay Fresh~\'', prompt]
},
{
env: { NODE_REPL_HISTORY_FILE: oldHistoryPath,
Expand Down