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.

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 – Add "magic" mode detection, persistent history support #1513

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions doc/api/readline.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ the following values:
treated like a TTY, and have ANSI/VT100 escape codes written to it.
Defaults to checking `isTTY` on the `output` stream upon instantiation.

- `historySize` - maximum number of history lines retained. Defaults to `30`.
Copy link
Member

Choose a reason for hiding this comment

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

Memory is cheap. Why not 300 or 3000?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original constant was 30. I can change it if desired?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, 30 is pretty small

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this option at 30 so as not to modify existing userland repls. The internal CLI repl now defaults to 1000, though.


The `completer` function is given the current line entered by the user, and
is supposed to return an Array with 2 entries:

Expand Down
20 changes: 20 additions & 0 deletions doc/api/repl.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ For example, you could add this to your bashrc file:

alias iojs="env NODE_NO_READLINE=1 rlwrap iojs"

The built-in repl (invoked by running `iojs` or `iojs -i`) may be controlled
via the following environment variables:

- `NODE_REPL_HISTORY_FILE` - if given, must be a path to a user-writable,
user-readable file. When a valid path is given, persistent history support
is enabled: REPL history will persist across `iojs` repl sessions.
- `NODE_REPL_HISTORY_SIZE` - defaults to `1000`. In conjunction with
`NODE_REPL_HISTORY_FILE`, controls how many lines of history will be
persisted. Must be a positive number.
- `NODE_REPL_MODE` - may be any of `sloppy`, `strict`, or `magic`. Defaults
to `magic`, which will automatically run "strict mode only" statements in
strict mode.

## repl.start(options)

Expand Down Expand Up @@ -64,6 +76,14 @@ the following values:
returns the formatting (including coloring) to display. Defaults to
`util.inspect`.

- `replMode` - controls whether the repl runs all commands in strict mode,
default mode, or a hybrid mode ("magic" mode.) Acceptable values are:
* `repl.REPL_MODE_SLOPPY` - run commands in sloppy mode.
* `repl.REPL_MODE_STRICT` - run commands in strict mode. This is equivalent to
prefacing every repl statement with `'use strict'`.
* `repl.REPL_MODE_MAGIC` - attempt to run commands in default mode. If they
fail to parse, re-try in strict mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put a note that strict mode is the same as 'use strict'? Also should probably not capitalize 'Run' and 'Attempt' here.


You can use your own `eval` function if it has following signature:

function eval(cmd, context, filename, callback) {
Expand Down
168 changes: 168 additions & 0 deletions lib/internal/repl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
'use strict';

module.exports = {createRepl: createRepl};

const Interface = require('readline').Interface;
const REPL = require('repl');
const path = require('path');

// XXX(chrisdickinson): The 15ms debounce value is somewhat arbitrary.
// The debounce is to guard against code pasted into the REPL.
const kDebounceHistoryMS = 15;

try {
// hack for require.resolve("./relative") to work properly.
module.filename = path.resolve('repl');
} catch (e) {
// path.resolve('repl') fails when the current working directory has been
// deleted. Fall back to the directory name of the (absolute) executable
// path. It's not really correct but what are the alternatives?
const dirname = path.dirname(process.execPath);
module.filename = path.resolve(dirname, 'repl');
}

// hack for repl require to work properly with node_modules folders
module.paths = require('module')._nodeModulePaths(module.filename);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hack moved here from lib/repl.js.


function createRepl(env, cb) {
const opts = {
useGlobal: true,
ignoreUndefined: false
};

if (parseInt(env.NODE_NO_READLINE)) {
opts.terminal = false;
}
if (parseInt(env.NODE_DISABLE_COLORS)) {
opts.useColors = false;
}

opts.replMode = {
'strict': REPL.REPL_MODE_STRICT,
'sloppy': REPL.REPL_MODE_SLOPPY,
'magic': REPL.REPL_MODE_MAGIC
}[String(env.NODE_REPL_MODE).toLowerCase().trim()];

if (opts.replMode === undefined) {
opts.replMode = REPL.REPL_MODE_MAGIC;
}

const historySize = Number(env.NODE_REPL_HISTORY_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, this allows for things like NODE_REPL_HISTORY_SIZE=-Infinity. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infinity should be allowed, but negative numbers should be disallowed.

if (!isNaN(historySize) && historySize > 0) {
opts.historySize = historySize;
} else {
// XXX(chrisdickinson): set here to avoid affecting existing applications
// using repl instances.
opts.historySize = 1000;
}

const repl = REPL.start(opts);
if (env.NODE_REPL_HISTORY_PATH) {
return setupHistory(repl, env.NODE_REPL_HISTORY_PATH, cb);
Copy link
Member

Choose a reason for hiding this comment

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

NODE_REPL_HISTORY_PATH seems dangerous in combination with setuid root applications.

I would suggest skipping the history when the effective UID is not the real user UID (and ditto for the GID). We don't have bindings for geteuid() and getegid() at the moment, those would have to be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From our conversation in IRC, I think I'm going to punt on this – a setuid'd binary running a repl will be dangerous no matter what; I think the better solution is to detect those situations and refuse to run the REPL at all (or, if we can get away with it, the entire program!) Once #1536 lands we should be able to open a new PR introducing those changes.

}
repl._historyPrev = _replHistoryMessage;
cb(null, repl);
}

function setupHistory(repl, historyPath, ready) {
const fs = require('fs');
var timer = null;
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);
}

function onread(err, data) {
if (err) {
return ready(err);
}

if (data) {
try {
repl.history = JSON.parse(data);
if (!Array.isArray(repl.history)) {
throw new Error('Expected array, got ' + typeof repl.history);
}
repl.history.slice(-repl.historySize);
Copy link
Member

Choose a reason for hiding this comment

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

Yields unexpected results when historySize <= 0.

} catch (err) {
return ready(
new Error(`Could not parse history data in ${historyPath}.`));
}
}

fs.open(historyPath, 'w', onhandle);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why we can't just have a+ fd? Because of multiple repl instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need to trim the output to historySize.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. A question: if multiple repls will be opened simultaneously, which of them will "win"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last one that entered a line will win.

}

function onhandle(err, hnd) {
if (err) {
return ready(err);
}
repl._historyHandle = hnd;
repl.on('line', online);
repl.resume();
return ready(null, repl);
}

// ------ history listeners ------
function online() {
repl._flushing = true;

if (timer) {
clearTimeout(timer);
}

timer = setTimeout(flushHistory, kDebounceHistoryMS);
}

function flushHistory() {
timer = null;
if (writing) {
pending = true;
return;
}
writing = true;
const historyData = JSON.stringify(repl.history, null, 2);
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');
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior as written is to attempt to commit the history to disk 15ms after the last input. This means that multiple clis will clobber each others persistent histories (the last one to exit "wins".)

Copy link
Member

Choose a reason for hiding this comment

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

what's the significance of 15ms? too short to be a user-debounce and the number seems arbitrary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to guard against code pasted into the repl. I agree the number is a bit arbitrary. I'll add a comment explaining as much so that if we have to change it folks'll know it's okay.

}


function _replHistoryMessage() {
if (this.history.length === 0) {
this._writeToOutput(
'\nPersistent history support disabled. ' +
'Set the NODE_REPL_HISTORY_PATH environment variable to ' +
'a valid, user-writable path to enable.\n'
);
this._refreshLine();
}
this._historyPrev = Interface.prototype._historyPrev;
return this._historyPrev();
}
21 changes: 12 additions & 9 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,17 @@ Module._load = function(request, parent, isMain) {
debug('Module._load REQUEST ' + (request) + ' parent: ' + parent.id);
}

// REPL is a special case, because it needs the real require.
if (request === 'internal/repl' || request === 'repl') {
if (Module._cache[request]) {
return Module._cache[request];
}
var replModule = new Module(request);
replModule._compile(NativeModule.getSource(request), `${request}.js`);
NativeModule._cache[request] = replModule;
return replModule.exports;
}

var filename = Module._resolveFilename(request, parent);

var cachedModule = Module._cache[filename];
Expand All @@ -281,14 +292,6 @@ Module._load = function(request, parent, isMain) {
}

if (NativeModule.nonInternalExists(filename)) {
// REPL is a special case, because it needs the real require.
if (filename == 'repl') {
var replModule = new Module('repl');
replModule._compile(NativeModule.getSource('repl'), 'repl.js');
NativeModule._cache.repl = replModule;
return replModule.exports;
}

debug('load native module ' + request);
return NativeModule.require(filename);
}
Expand Down Expand Up @@ -502,7 +505,7 @@ Module._initPaths = function() {

// bootstrap repl
Module.requireRepl = function() {
return Module._load('repl', '.');
return Module._load('internal/repl', '.');
};

Module._initPaths();
Expand Down
12 changes: 11 additions & 1 deletion lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,30 @@ function Interface(input, output, completer, terminal) {
this._sawReturn = false;

EventEmitter.call(this);
var historySize;

if (arguments.length === 1) {
// an options object was given
output = input.output;
completer = input.completer;
terminal = input.terminal;
historySize = input.historySize;
Copy link
Member

Choose a reason for hiding this comment

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

I mean let's leave this here, but remove the argument.

input = input.input;
}
historySize = historySize || kHistorySize;

completer = completer || function() { return []; };

if (typeof completer !== 'function') {
throw new TypeError('Argument \'completer\' must be a function');
}

if (typeof historySize !== 'number' ||
isNaN(historySize) ||
historySize < 0) {
throw new TypeError('Argument \'historySize\' must be a positive number');
}

// backwards compat; check the isTTY prop of the output stream
// when `terminal` was not specified
if (terminal === undefined && !(output === null || output === undefined)) {
Expand All @@ -60,6 +69,7 @@ function Interface(input, output, completer, terminal) {

this.output = output;
this.input = input;
this.historySize = historySize;

// Check arity, 2 - for async, 1 for sync
this.completer = completer.length === 2 ? completer : function(v, callback) {
Expand Down Expand Up @@ -214,7 +224,7 @@ Interface.prototype._addHistory = function() {
this.history.unshift(this.line);

// Only store so many
if (this.history.length > kHistorySize) this.history.pop();
if (this.history.length > this.historySize) this.history.pop();
}

this.historyIndex = -1;
Expand Down
Loading