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: don't use deprecated domain module #55204

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
7 changes: 0 additions & 7 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -2021,13 +2021,6 @@ An invalid `options.protocol` was passed to `http.request()`.
Both `breakEvalOnSigint` and `eval` options were set in the [`REPL`][] config,
which is not supported.

<a id="ERR_INVALID_REPL_INPUT"></a>

### `ERR_INVALID_REPL_INPUT`

The input may not be used in the [`REPL`][]. The conditions under which this
error is used are described in the [`REPL`][] documentation.

Comment on lines -2024 to -2030
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this to the ad-hoc section

node/doc/api/errors.md

Lines 3257 to 3260 in cdae315

## Legacy Node.js error codes
> Stability: 0 - Deprecated. These error codes are either inconsistent, or have
> been removed.

<a id="ERR_INVALID_RETURN_PROPERTY"></a>

### `ERR_INVALID_RETURN_PROPERTY`
Expand Down
32 changes: 6 additions & 26 deletions doc/api/repl.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,34 +151,18 @@ global or scoped variable, the input `fs` will be evaluated on-demand as

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55204
description: The `process.setUncaughtExceptionCaptureCallback()`
Copy link
Member

Choose a reason for hiding this comment

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

This is an implementation detail. Does it change the behavior of the function? If not, we should remove it from here. If it changes the behavior, this description should mention the behavior change rather than implementation specifics.

function is now used instead of the `domain` module.
- version: v12.3.0
pr-url: https://github.com/nodejs/node/pull/27151
description: The `'uncaughtException'` event is from now on triggered if the
repl is used as standalone program.
-->

The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
REPL session.

This use of the [`domain`][] module in the REPL has these side effects:

* Uncaught exceptions only emit the [`'uncaughtException'`][] event in the
standalone REPL. Adding a listener for this event in a REPL within
another Node.js program results in [`ERR_INVALID_REPL_INPUT`][].

```js
const r = repl.start();

r.write('process.on("uncaughtException", () => console.log("Foobar"));\n');
// Output stream includes:
// TypeError [ERR_INVALID_REPL_INPUT]: Listeners for `uncaughtException`
// cannot be used in the REPL

r.close();
```

* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.
The REPL uses the [`process.setUncaughtExceptionCaptureCallback()`][] to catch all uncaught exceptions for that
REPL session. For more information on potential side effects, refer to it's documentation.

#### Assignment of the `_` (underscore) variable

Expand Down Expand Up @@ -768,12 +752,8 @@ avoiding open network interfaces.

[TTY keybindings]: readline.md#tty-keybindings
[ZSH]: https://en.wikipedia.org/wiki/Z_shell
[`'uncaughtException'`]: process.md#event-uncaughtexception
[`--no-experimental-repl-await`]: cli.md#--no-experimental-repl-await
[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.md#err_domain_cannot_set_uncaught_exception_capture
[`ERR_INVALID_REPL_INPUT`]: errors.md#err_invalid_repl_input
[`curl(1)`]: https://curl.haxx.se/docs/manpage.html
[`domain`]: domain.md
[`process.setUncaughtExceptionCaptureCallback()`]: process.md#processsetuncaughtexceptioncapturecallbackfn
[`readline.InterfaceCompleter`]: readline.md#use-of-the-completer-function
[`repl.ReplServer`]: #class-replserver
Expand Down
1 change: 0 additions & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1502,7 +1502,6 @@ E('ERR_INVALID_PROTOCOL',
TypeError);
E('ERR_INVALID_REPL_EVAL_CONFIG',
'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
E('ERR_INVALID_REPL_INPUT', '%s', TypeError);
E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
return `Expected a valid ${input} to be returned for the "${prop}" from the` +
` "${name}" hook but got ${determineSpecificType(value)}.`;
Expand Down
82 changes: 27 additions & 55 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const {
ArrayPrototypeUnshift,
Boolean,
Error: MainContextError,
FunctionPrototypeApply,
FunctionPrototypeBind,
JSONStringify,
MathMaxApply,
Expand All @@ -78,7 +79,6 @@ const {
RegExpPrototypeExec,
SafePromiseRace,
SafeSet,
SafeWeakSet,
StringPrototypeCharAt,
StringPrototypeCodePointAt,
StringPrototypeEndsWith,
Expand Down Expand Up @@ -138,7 +138,6 @@ ArrayPrototypeForEach(
BuiltinModule.getSchemeOnlyModuleNames(),
(lib) => ArrayPrototypePush(nodeSchemeBuiltinLibs, `node:${lib}`),
);
const domain = require('domain');
let debug = require('internal/util/debuglog').debuglog('repl', (fn) => {
debug = fn;
});
Expand All @@ -147,7 +146,6 @@ const {
codes: {
ERR_CANNOT_WATCH_SIGINT,
ERR_INVALID_REPL_EVAL_CONFIG,
ERR_INVALID_REPL_INPUT,
ERR_MISSING_ARGS,
ERR_SCRIPT_EXECUTION_INTERRUPTED,
},
Expand Down Expand Up @@ -205,14 +203,11 @@ const globalBuiltins =
new SafeSet(vm.runInNewContext('Object.getOwnPropertyNames(globalThis)'));

const parentModule = module;
const domainSet = new SafeWeakSet();

const kBufferedCommandSymbol = Symbol('bufferedCommand');
const kContextId = Symbol('contextId');
const kLoadingSymbol = Symbol('loading');

let addedNewListener = false;

try {
// Hack for require.resolve("./relative") to work properly.
module.filename = path.resolve('repl');
Expand Down Expand Up @@ -347,7 +342,6 @@ function REPLServer(prompt,

this.allowBlockingCompletions = !!options.allowBlockingCompletions;
this.useColors = !!options.useColors;
this._domain = options.domain || domain.create();
this.useGlobal = !!useGlobal;
this.ignoreUndefined = !!ignoreUndefined;
this.replMode = replMode || module.exports.REPL_MODE_SLOPPY;
Expand All @@ -370,27 +364,10 @@ function REPLServer(prompt,
// It is possible to introspect the running REPL accessing this variable
// from inside the REPL. This is useful for anyone working on the REPL.
module.exports.repl = this;
} else if (!addedNewListener) {
// Add this listener only once and use a WeakSet that contains the REPLs
// domains. Otherwise we'd have to add a single listener to each REPL
// instance and that could trigger the `MaxListenersExceededWarning`.
process.prependListener('newListener', (event, listener) => {
if (event === 'uncaughtException' &&
process.domain &&
listener.name !== 'domainUncaughtExceptionClear' &&
domainSet.has(process.domain)) {
// Throw an error so that the event will not be added and the current
// domain takes over. That way the user is notified about the error
// and the current code evaluation is stopped, just as any other code
// that contains an error.
throw new ERR_INVALID_REPL_INPUT(
'Listeners for `uncaughtException` cannot be used in the REPL');
}
});
addedNewListener = true;
}

domainSet.add(this._domain);
process.setUncaughtExceptionCaptureCallback(null);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we call it twice? Please add a comment

process.setUncaughtExceptionCaptureCallback(handleEvaluationError);

const savedRegExMatches = ['', '', '', '', '', '', '', '', '', ''];
const sep = '\u0000\u0000\u0000';
Expand Down Expand Up @@ -613,13 +590,8 @@ function REPLServer(prompt,
}
} catch (e) {
err = e;

if (process.domain) {
debug('not recoverable, send to domain');
process.domain.emit('error', err);
process.domain.exit();
return;
}
handleEvaluationError(e);
return;
}

if (awaitPromise && !err) {
Expand All @@ -645,10 +617,8 @@ function REPLServer(prompt,
const result = (await promise)?.value;
finishExecution(null, result);
} catch (err) {
if (err && process.domain) {
debug('not recoverable, send to domain');
process.domain.emit('error', err);
process.domain.exit();
if (err) {
handleEvaluationError(err);
return;
}
finishExecution(err);
Expand All @@ -666,10 +636,16 @@ function REPLServer(prompt,
}
}

self.eval = self._domain.bind(eval_);
self.eval = function(...args) {
try {
FunctionPrototypeApply(eval_, this, args);
} catch (e) {
handleEvaluationError(e);
}
};

self._domain.on('error', function debugDomainError(e) {
debug('domain error');
function handleEvaluationError(e) {
debug('eval error');
let errStack = '';

if (typeof e === 'object' && e !== null) {
Expand Down Expand Up @@ -697,11 +673,6 @@ function REPLServer(prompt,
});
decorateErrorStack(e);

if (e.domainThrown) {
delete e.domain;
delete e.domainThrown;
}

if (isError(e)) {
if (e.stack) {
if (e.name === 'SyntaxError') {
Expand Down Expand Up @@ -779,7 +750,7 @@ function REPLServer(prompt,
self.lines.level = [];
self.displayPrompt();
}
});
};

self.clearBufferedCommand();

Expand Down Expand Up @@ -952,7 +923,7 @@ function REPLServer(prompt,
self.displayPrompt();
return;
}
self._domain.emit('error', e.err || e);
handleEvaluationError(e.err || e);
}

// Clear buffer if no SyntaxErrors
Expand All @@ -972,8 +943,7 @@ function REPLServer(prompt,
self.output.write(self.writer(ret) + '\n');
}

// Display prompt again (unless we already did by emitting the 'error'
// event on the domain instance).
// Display prompt again
if (!e) {
self.displayPrompt();
}
Expand Down Expand Up @@ -1083,15 +1053,17 @@ REPLServer.prototype.clearBufferedCommand = function clearBufferedCommand() {
REPLServer.prototype.close = function close() {
if (this.terminal && this._flushing && !this._closingOnFlush) {
this._closingOnFlush = true;
this.once('flushHistory', () =>
ReflectApply(Interface.prototype.close, this, []),
);
this.once('flushHistory', () => {
process.setUncaughtExceptionCaptureCallback(null);
ReflectApply(Interface.prototype.close, this, []);
});

return;
}
process.nextTick(() =>
ReflectApply(Interface.prototype.close, this, []),
);
process.nextTick(() => {
process.setUncaughtExceptionCaptureCallback(null);
ReflectApply(Interface.prototype.close, this, []);
});
};

REPLServer.prototype.createContext = function() {
Expand Down
8 changes: 4 additions & 4 deletions test/fixtures/repl-tab-completion-nested-repls.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ 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;
const testMe = repl.start({
prompt: '',
input: putIn,
output: process.stdout,
});

// Nesting of structures causes REPL to use a nested REPL for completion.
Expand Down
45 changes: 0 additions & 45 deletions test/parallel/test-repl-domain.js

This file was deleted.

7 changes: 0 additions & 7 deletions test/parallel/test-repl-save-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ const works = [['inner.one'], 'inner.o'];
const putIn = new ArrayStream();
const testMe = repl.start('', putIn);

// Some errors might be passed to the domain.
testMe._domain.on('error', function(reason) {
Copy link
Member

Choose a reason for hiding this comment

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

This will require CITGM since you're altering a publicly accessible variable.

const err = new Error('Test failed');
err.reason = reason;
throw err;
});

const testFile = [
'let inner = (function() {',
' return {one:1};',
Expand Down
3 changes: 0 additions & 3 deletions test/parallel/test-repl-tab-complete-import.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ const testMe = repl.start({
allowBlockingCompletions: true
});

// Some errors are passed to the domain, but do not callback
testMe._domain.on('error', assert.ifError);

// Tab complete provides built in libs for import()
testMe.complete('import(\'', common.mustCall((error, data) => {
assert.strictEqual(error, null);
Expand Down
4 changes: 1 addition & 3 deletions test/parallel/test-repl-tab-complete-nested-repls.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,4 @@ 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');
const err = result.stderr.toString();
assert.ok(err.includes('fhqwhgads'), err);
assert.ok(result.stdout.toString().includes('fhqwhgads'));
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this test?

3 changes: 0 additions & 3 deletions test/parallel/test-repl-tab-complete.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,6 @@ const testMe = repl.start({
allowBlockingCompletions: true
});

// Some errors are passed to the domain, but do not callback
testMe._domain.on('error', assert.ifError);

// Tab Complete will not break in an object literal
putIn.run([
'var inner = {',
Expand Down
4 changes: 1 addition & 3 deletions test/parallel/test-repl-tab.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';
const common = require('../common');
require('../common');
const assert = require('assert');
const repl = require('repl');
const zlib = require('zlib');
Expand All @@ -11,8 +11,6 @@ const testMe = repl.start('', putIn, function(cmd, context, filename,
callback(null, cmd);
});

testMe._domain.on('error', common.mustNotCall());
Copy link
Member

Choose a reason for hiding this comment

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

This needs an equivalent change.


testMe.complete('', function(err, results) {
assert.strictEqual(err, null);
});
Loading
Loading