-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Breaking change in V8 for Error.prepareStackTrace #21574
Comments
This comment has been minimized.
This comment has been minimized.
@Trott Error.prepareStackTrace is a special v8 thing that isn't in any spec. |
This comment has been minimized.
This comment has been minimized.
Does this behavior change look correct/acceptable? Code: 'use strict';
const repl = require('repl');
const r = repl.start({
prompt: '',
terminal: false,
useColors: false
});
r.write('throw new Error("whoops");\n');
r.close(); Output currently: Error: whoops Output after this V8 change lands: Error: whoops
at repl:1:7
at Script.runInContext (vm.js:100:20)
at REPLServer.defaultEval (repl.js:319:29)
at bound (domain.js:396:14)
at REPLServer.runBound [as eval] (domain.js:409:12)
at REPLServer.onLine (repl.js:615:10)
at REPLServer.emit (events.js:182:13)
at REPLServer.EventEmitter.emit (domain.js:442:20)
at REPLServer.Interface._onLine (readline.js:290:10)
at REPLServer.Interface._normalWrite (readline.js:433:12) If that looks correct, I'll modify the tests to accommodate both the current and changed behavior. If the new output is deemed undesirable, then this will require some work on the REPL code rather than the tests. @nodejs/repl |
It seems like this was done quite intentionally: https://github.com/nodejs/node/blob/master/lib/repl.js#L491-L513 (c5f54b1) I haven’t tried it, but maybe instead of mutating |
Not sure if it changes anything, but FWIW, as best as I can tell, these additional stack trace lines only appear when using the REPL programmatically. Interactive REPL behavior seems unchanged. (I could be wrong, but that's what I'm seeing.) |
/ping @lance |
@Trott That might be because for the buit-in REPL, we use Node’s main Context rather than creating a new one (i.e. |
@lance Any chance you want to grab the V8 patch and see about repairing the stacktrace pruning feature in the REPL? |
FWIW, this fixed the diff --git a/lib/repl.js b/lib/repl.js
index 6b5f480387..0de6cfd0ff 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -399,11 +399,12 @@ function REPLServer(prompt,
self._domain.on('error', function debugDomainError(e) {
debug('domain error');
const top = replMap.get(self);
- const pstrace = Error.prepareStackTrace;
- Error.prepareStackTrace = prepareStackTrace(pstrace);
+ const originalPrepareStackTrace = e.constructor.prepareStackTrace;
+ e.constructor.prepareStackTrace =
+ prepareStackTrace(Error.prepareStackTrace);
if (typeof e === 'object')
internalUtil.decorateErrorStack(e);
- Error.prepareStackTrace = pstrace;
+ Error.prepareStackTrace = originalPrepareStackTrace;
const isError = internalUtil.isError(e);
if (!self.underscoreErrAssigned)
self.lastError = e; |
Is there any progress on this? I'd like to go ahead and land the V8 patch soonish. |
I'll take a look today after tc39 if no one else gets to it by then... should just be looking up Error from the repl's context instead of global |
@devsnek any news? |
@schuay we're gonna need a magic way to get the context of an error's constructor for this to work in node. i think its worth pursuing https://crrev.com/c/1119768 as the solution for node. the following would probably work but i would rather not do it void RealmGlobalForObject(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(args[0]->CreationContext()->Global());
} realmGlobalForObject(error).Error.prepareStackTrace |
There's been no further activity on this in nearly 2 years. Does this need to remain open? |
This was handled a while back in b046bd1 |
The proposed change (not yet landed): https://crrev.com/c/1113438
And the original related node issue: #21270
This changes formatting behavior for stack traces to use the
Error.prepareStackTrace
function set in the original creation context of the error object, instead of the current context.The following node tests start failing:
parallel/test-repl-pretty-custom-stack
parallel/test-repl-pretty-stack
parallel/test-repl-underscore
Failures look similar to:
Full test run logs: https://logs.chromium.org/v/?s=v8%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8942571998063714288%2F%2B%2Fsteps%2Frun_tests%2F0%2Fstdout
Presumably this happens because custom stack formatting is set in one context but not another.
Can this reasonably be fixed in node?
The text was updated successfully, but these errors were encountered: