-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
repl: don't use deprecated domain module
#55204
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
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 |
|---|---|---|
|
|
@@ -147,39 +147,6 @@ global or scoped variable, the input `fs` will be evaluated on-demand as | |
| > fs.createReadStream('./some/file'); | ||
| ``` | ||
|
|
||
| #### Global uncaught exceptions | ||
|
|
||
| <!-- YAML | ||
| changes: | ||
| - 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`][]. | ||
|
Member
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. I'd replace this section with one that describes the new behavior and amend the
Member
Author
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. I don't think a specific section is needed, IMO there's nothing to document. it adds a listener to the process object, what negative impact does that have?
Member
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. @redyetidev Well, before this change,
Member
Author
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. That's incorrect. Before this change, uncaughtException listeners still saw REPL exceptions
Member
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. Oops. Nevermind then!
Member
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. I am confused about this change. As far as I see it in the code, only the stand alone repl works with the uncaughtException listeners. In case more than a single REPL instance is running, it is not possible to identify the correct listeners, so none receive the errors. |
||
|
|
||
| ```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. | ||
|
|
||
| #### Assignment of the `_` (underscore) variable | ||
|
|
||
| <!-- YAML | ||
|
|
@@ -768,13 +735,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 | ||
| [`repl.start()`]: #replstartoptions | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -60,6 +60,7 @@ const { | |||||
| ArrayPrototypeUnshift, | ||||||
| Boolean, | ||||||
| Error: MainContextError, | ||||||
| FunctionPrototypeApply, | ||||||
| FunctionPrototypeBind, | ||||||
| JSONStringify, | ||||||
| MathMaxApply, | ||||||
|
|
@@ -76,9 +77,9 @@ const { | |||||
| ReflectApply, | ||||||
| RegExp, | ||||||
| RegExpPrototypeExec, | ||||||
| SafeMap, | ||||||
| SafePromiseRace, | ||||||
| SafeSet, | ||||||
| SafeWeakSet, | ||||||
| StringPrototypeCharAt, | ||||||
| StringPrototypeCodePointAt, | ||||||
| StringPrototypeEndsWith, | ||||||
|
|
@@ -138,7 +139,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 +147,6 @@ const { | |||||
| codes: { | ||||||
| ERR_CANNOT_WATCH_SIGINT, | ||||||
| ERR_INVALID_REPL_EVAL_CONFIG, | ||||||
| ERR_INVALID_REPL_INPUT, | ||||||
| ERR_MISSING_ARGS, | ||||||
| ERR_SCRIPT_EXECUTION_INTERRUPTED, | ||||||
| }, | ||||||
|
|
@@ -191,6 +190,7 @@ const { | |||||
| const { | ||||||
| makeContextifyScript, | ||||||
| } = require('internal/vm'); | ||||||
| const { createHook } = require('async_hooks'); | ||||||
| let nextREPLResourceNumber = 1; | ||||||
| // This prevents v8 code cache from getting confused and using a different | ||||||
| // cache from a resource of the same name | ||||||
|
|
@@ -205,13 +205,48 @@ 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'); | ||||||
| const kListeningREPLs = new SafeSet(); | ||||||
| const kAsyncREPLMap = new SafeMap(); | ||||||
| let kActiveREPL; | ||||||
|
Member
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.
Suggested change
|
||||||
| const kAsyncHook = createHook({ | ||||||
| init(asyncId) { | ||||||
| if (kActiveREPL) { | ||||||
| kAsyncREPLMap.set(asyncId, kActiveREPL); | ||||||
| } | ||||||
| }, | ||||||
|
|
||||||
| before(asyncId) { | ||||||
| kActiveREPL = kAsyncREPLMap.get(asyncId) || kActiveREPL; | ||||||
| }, | ||||||
|
|
||||||
| destroy(asyncId) { | ||||||
| kAsyncREPLMap.delete(asyncId); | ||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| let kHasSetUncaughtListener = false; | ||||||
|
Member
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 not a constant, so I would name the variable different.
Suggested change
|
||||||
|
|
||||||
| function handleUncaughtException(er) { | ||||||
| if (kActiveREPL) { | ||||||
| kActiveREPL._onEvalError(er); | ||||||
|
Comment on lines
+234
to
+235
Member
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. Are we definitely safe, that the active repl is definitely the correct one? |
||||||
| // If there are no other event listeners, throw the uncaught exception. | ||||||
| } else if (process.listenerCount('uncaughtException') <= 1) { | ||||||
| throw er; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| let addedNewListener = false; | ||||||
| function removeListeningREPL(repl) { | ||||||
| kListeningREPLs.delete(repl); | ||||||
| if (kListeningREPLs.size === 0) { | ||||||
| kAsyncHook.disable(); | ||||||
| kHasSetUncaughtListener = false; | ||||||
| process.off('uncaughtException', handleUncaughtException); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| // Hack for require.resolve("./relative") to work properly. | ||||||
|
|
@@ -346,7 +381,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; | ||||||
|
|
@@ -369,28 +403,8 @@ 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); | ||||||
|
|
||||||
| const savedRegExMatches = ['', '', '', '', '', '', '', '', '', '']; | ||||||
| const sep = '\u0000\u0000\u0000'; | ||||||
| const regExMatcher = new RegExp(`^${sep}(.*)${sep}(.*)${sep}(.*)${sep}(.*)` + | ||||||
|
|
@@ -612,13 +626,8 @@ function REPLServer(prompt, | |||||
| } | ||||||
| } catch (e) { | ||||||
| err = e; | ||||||
|
Member
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. Nit: the assignment is AFAIK not needed anymore |
||||||
|
|
||||||
| if (process.domain) { | ||||||
| debug('not recoverable, send to domain'); | ||||||
| process.domain.emit('error', err); | ||||||
| process.domain.exit(); | ||||||
| return; | ||||||
| } | ||||||
| self._onEvalError(e); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| if (awaitPromise && !err) { | ||||||
|
|
@@ -644,10 +653,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) { | ||||||
| self._onEvalError(err); | ||||||
| return; | ||||||
| } | ||||||
| finishExecution(err); | ||||||
|
|
@@ -665,10 +672,24 @@ function REPLServer(prompt, | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| self.eval = self._domain.bind(eval_); | ||||||
| self.eval = function(...args) { | ||||||
| kActiveREPL = this; | ||||||
|
|
||||||
| const cb = args[3]; | ||||||
| args[3] = (...cbArgs) => { | ||||||
| kActiveREPL = null; | ||||||
| FunctionPrototypeApply(cb, null, cbArgs); | ||||||
| }; | ||||||
|
|
||||||
| self._domain.on('error', function debugDomainError(e) { | ||||||
| debug('domain error'); | ||||||
| try { | ||||||
| FunctionPrototypeApply(eval_, this, args); | ||||||
|
Member
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.
|
||||||
| } catch (e) { | ||||||
| self._onEvalError(e); | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
| self._onEvalError = function _onEvalError(e) { | ||||||
|
Member
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. It would be great not to expose this property. For testing purposes, we would only have to verify that nothing else is written to |
||||||
| debug('eval error'); | ||||||
| let errStack = ''; | ||||||
|
|
||||||
| if (typeof e === 'object' && e !== null) { | ||||||
|
|
@@ -696,11 +717,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') { | ||||||
|
|
@@ -740,10 +756,13 @@ function REPLServer(prompt, | |||||
| self.lastError = e; | ||||||
| } | ||||||
|
|
||||||
| if (options[kStandaloneREPL] && | ||||||
| process.listenerCount('uncaughtException') !== 0) { | ||||||
| if (options[kStandaloneREPL] && process.listenerCount('uncaughtException') > 1) { | ||||||
| process.nextTick(() => { | ||||||
| process.emit('uncaughtException', e); | ||||||
| const listeners = process.listeners('uncaughtException'); | ||||||
| for (let i = 0; i < listeners.length; i++) { | ||||||
| const listener = listeners[i]; | ||||||
| if (listener !== handleUncaughtException) listener(e); | ||||||
| } | ||||||
| self.clearBufferedCommand(); | ||||||
| self.lines.level = []; | ||||||
| self.displayPrompt(); | ||||||
|
|
@@ -778,7 +797,14 @@ function REPLServer(prompt, | |||||
| self.lines.level = []; | ||||||
| self.displayPrompt(); | ||||||
| } | ||||||
| }); | ||||||
| }; | ||||||
| kListeningREPLs.add(self); | ||||||
|
|
||||||
| if (!kHasSetUncaughtListener) { | ||||||
| kAsyncHook.enable(); | ||||||
| process.on('uncaughtException', handleUncaughtException); | ||||||
avivkeller marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| kHasSetUncaughtListener = true; | ||||||
| } | ||||||
|
|
||||||
| self.clearBufferedCommand(); | ||||||
|
|
||||||
|
|
@@ -951,7 +977,7 @@ function REPLServer(prompt, | |||||
| self.displayPrompt(); | ||||||
| return; | ||||||
| } | ||||||
| self._domain.emit('error', e.err || e); | ||||||
| self._onEvalError(e.err || e); | ||||||
| } | ||||||
|
|
||||||
| // Clear buffer if no SyntaxErrors | ||||||
|
|
@@ -971,8 +997,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(); | ||||||
| } | ||||||
|
|
@@ -1082,15 +1107,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', () => { | ||||||
| removeListeningREPL(this); | ||||||
| ReflectApply(Interface.prototype.close, this, []); | ||||||
| }); | ||||||
|
|
||||||
| return; | ||||||
| } | ||||||
| process.nextTick(() => | ||||||
| ReflectApply(Interface.prototype.close, this, []), | ||||||
| ); | ||||||
| process.nextTick(() => { | ||||||
| removeListeningREPL(this); | ||||||
| ReflectApply(Interface.prototype.close, this, []); | ||||||
| }); | ||||||
| }; | ||||||
|
|
||||||
| REPLServer.prototype.createContext = function() { | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.