-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ const { | |
ArrayPrototypeUnshift, | ||
Boolean, | ||
Error: MainContextError, | ||
FunctionPrototypeApply, | ||
FunctionPrototypeBind, | ||
JSONStringify, | ||
MathMaxApply, | ||
|
@@ -78,7 +79,6 @@ const { | |
RegExpPrototypeExec, | ||
SafePromiseRace, | ||
SafeSet, | ||
SafeWeakSet, | ||
StringPrototypeCharAt, | ||
StringPrototypeCodePointAt, | ||
StringPrototypeEndsWith, | ||
|
@@ -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; | ||
}); | ||
|
@@ -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, | ||
}, | ||
|
@@ -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'); | ||
|
@@ -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; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
|
@@ -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) { | ||
|
@@ -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); | ||
|
@@ -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) { | ||
|
@@ -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') { | ||
|
@@ -779,7 +750,7 @@ function REPLServer(prompt, | |
self.lines.level = []; | ||
self.displayPrompt(); | ||
} | ||
}); | ||
}; | ||
|
||
self.clearBufferedCommand(); | ||
|
||
|
@@ -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 | ||
|
@@ -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(); | ||
} | ||
|
@@ -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() { | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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};', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you change this test? |
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'); | ||
|
@@ -11,8 +11,6 @@ const testMe = repl.start('', putIn, function(cmd, context, filename, | |
callback(null, cmd); | ||
}); | ||
|
||
testMe._domain.on('error', common.mustNotCall()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); |
There was a problem hiding this comment.
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