Skip to content

Commit 83d0404

Browse files
committed
repl: do not swallow errors in nested REPLs
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: #21586 PR-URL: #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>
1 parent b3b3f53 commit 83d0404

4 files changed

+73
-2
lines changed

lib/repl.js

+1
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,7 @@ function complete(line, callback) {
10021002
// all this is only profitable if the nested REPL
10031003
// does not have a bufferedCommand
10041004
if (!magic[kBufferedCommandSymbol]) {
1005+
magic._domain.on('error', (err) => { throw err; });
10051006
return magic.complete(line, callback);
10061007
}
10071008
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Tab completion sometimes uses a separate REPL instance under the hood.
2+
// That REPL instance has its own domain. Make sure domain errors trickle back
3+
// up to the main REPL.
4+
//
5+
// Ref: https://github.com/nodejs/node/issues/21586
6+
7+
'use strict';
8+
9+
const { Stream } = require('stream');
10+
const { inherits } = require('util');
11+
function noop() {}
12+
13+
// A stream to push an array into a REPL
14+
function ArrayStream() {
15+
this.run = function(data) {
16+
data.forEach((line) => {
17+
this.emit('data', `${line}\n`);
18+
});
19+
};
20+
}
21+
22+
inherits(ArrayStream, Stream);
23+
ArrayStream.prototype.readable = true;
24+
ArrayStream.prototype.writable = true;
25+
ArrayStream.prototype.pause = noop;
26+
ArrayStream.prototype.resume = noop;
27+
ArrayStream.prototype.write = noop;
28+
29+
const repl = require('repl');
30+
31+
const putIn = new ArrayStream();
32+
const testMe = repl.start('', putIn);
33+
34+
// Some errors are passed to the domain, but do not callback
35+
testMe._domain.on('error', function(err) {
36+
throw err;
37+
});
38+
39+
// Nesting of structures causes REPL to use a nested REPL for completion.
40+
putIn.run([
41+
'var top = function() {',
42+
'r = function test (',
43+
' one, two) {',
44+
'var inner = {',
45+
' one:1',
46+
'};'
47+
]);
48+
49+
// In Node.js 10.11.0, this next line will terminate the repl silently...
50+
testMe.complete('inner.o', () => { throw new Error('fhqwhgads'); });
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Tab completion sometimes uses a separate REPL instance under the hood.
2+
// That REPL instance has its own domain. Make sure domain errors trickle back
3+
// up to the main REPL.
4+
//
5+
// Ref: https://github.com/nodejs/node/issues/21586
6+
7+
'use strict';
8+
9+
require('../common');
10+
const fixtures = require('../common/fixtures');
11+
12+
const assert = require('assert');
13+
const { spawnSync } = require('child_process');
14+
15+
const testFile = fixtures.path('repl-tab-completion-nested-repls.js');
16+
const result = spawnSync(process.execPath, [testFile]);
17+
18+
// The spawned process will fail. In Node.js 10.11.0, it will fail silently. The
19+
// test here is to make sure that the error information bubbles up to the
20+
// calling process.
21+
assert.ok(result.status, 'testFile swallowed its error');

test/parallel/test-repl-tab-complete.js

+1-2
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,8 @@ putIn.run([
154154
' one:1',
155155
'};'
156156
]);
157-
// See: https://github.com/nodejs/node/issues/21586
158-
// testMe.complete('inner.o', getNoResultsFunction());
159157
testMe.complete('inner.o', common.mustCall(function(error, data) {
158+
assert.deepStrictEqual(data, works);
160159
}));
161160

162161
putIn.run(['.clear']);

0 commit comments

Comments
 (0)