Skip to content

Commit

Permalink
repl: do not swallow errors in nested REPLs
Browse files Browse the repository at this point in the history
For tab completion, a REPLServer instance will sometimes create another
REPLServer instance. If a callback is sent to the `.complete()` function
and that callback throws an error, it will be swallowed by the nested
REPLs domain. Re-throw the error so that processes don't silently exit
without any indication of an error (including a status code).

Fixes: nodejs#21586

PR-URL: nodejs#23004
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
Trott committed Oct 2, 2018
1 parent b3b3f53 commit 83d0404
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 2 deletions.
1 change: 1 addition & 0 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,7 @@ function complete(line, callback) {
// all this is only profitable if the nested REPL
// does not have a bufferedCommand
if (!magic[kBufferedCommandSymbol]) {
magic._domain.on('error', (err) => { throw err; });
return magic.complete(line, callback);
}
}
Expand Down
50 changes: 50 additions & 0 deletions test/fixtures/repl-tab-completion-nested-repls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Tab completion sometimes uses a separate REPL instance under the hood.
// That REPL instance has its own domain. Make sure domain errors trickle back
// up to the main REPL.
//
// Ref: https://github.com/nodejs/node/issues/21586

'use strict';

const { Stream } = require('stream');
const { inherits } = require('util');
function noop() {}

// A stream to push an array into a REPL
function ArrayStream() {
this.run = function(data) {
data.forEach((line) => {
this.emit('data', `${line}\n`);
});
};
}

inherits(ArrayStream, Stream);
ArrayStream.prototype.readable = true;
ArrayStream.prototype.writable = true;
ArrayStream.prototype.pause = noop;
ArrayStream.prototype.resume = noop;
ArrayStream.prototype.write = noop;

const repl = require('repl');

const putIn = new ArrayStream();
const testMe = repl.start('', putIn);

// Some errors are passed to the domain, but do not callback
testMe._domain.on('error', function(err) {
throw err;
});

// Nesting of structures causes REPL to use a nested REPL for completion.
putIn.run([
'var top = function() {',
'r = function test (',
' one, two) {',
'var inner = {',
' one:1',
'};'
]);

// In Node.js 10.11.0, this next line will terminate the repl silently...
testMe.complete('inner.o', () => { throw new Error('fhqwhgads'); });
21 changes: 21 additions & 0 deletions test/parallel/test-repl-tab-complete-nested-repls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Tab completion sometimes uses a separate REPL instance under the hood.
// That REPL instance has its own domain. Make sure domain errors trickle back
// up to the main REPL.
//
// Ref: https://github.com/nodejs/node/issues/21586

'use strict';

require('../common');
const fixtures = require('../common/fixtures');

const assert = require('assert');
const { spawnSync } = require('child_process');

const testFile = fixtures.path('repl-tab-completion-nested-repls.js');
const result = spawnSync(process.execPath, [testFile]);

// The spawned process will fail. In Node.js 10.11.0, it will fail silently. The
// test here is to make sure that the error information bubbles up to the
// calling process.
assert.ok(result.status, 'testFile swallowed its error');
3 changes: 1 addition & 2 deletions test/parallel/test-repl-tab-complete.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,8 @@ putIn.run([
' one:1',
'};'
]);
// See: https://github.com/nodejs/node/issues/21586
// testMe.complete('inner.o', getNoResultsFunction());
testMe.complete('inner.o', common.mustCall(function(error, data) {
assert.deepStrictEqual(data, works);
}));

putIn.run(['.clear']);
Expand Down

0 comments on commit 83d0404

Please sign in to comment.