intent to implement - rework of stack trace decoration

[devsnek]

This is an attempt to finally fix the inconsistent stack decoration in node core... hopefully future-facing enough for everyone.

the final code might not be js but the logic will look something like this:

  // Called for every stack trace in the isolate.
  // more consistency as a platform and no more
  // calling decorateErrorStack which
  // is super slow and inconsistent.
  // Using the V8 callback also means we don't need
  // to store the state of whether an exception has
  // been decorated, V8 does it for us.
  setPrepareStackTraceCallback(
    (global, shouldNotDecorate, safeToString, error, frames, message) => {

      // V8 engine prepareStackTrace polyfill
      // To be buffer-constructor-style-deprecated
      // if/when ECMA262 error stacks API is finalized.
      // Being able to explicitly bail here means we don't
      // run into double-decoration issues, like with my last
      // attempt to fix stack decoration.
      if (global.Error !== undefined &&
          typeof global.Error.prepareStackTrace === 'function') {
        return global.Error.prepareStackTrace(error, frames);
      }

      const errorString = safeToString(error);

      // checks for Symbol.for('node.error.decorate') === false
      // or an internal weakset for core things (iirc there were one
      // or two places where we disable the arrow so i threw this in)
      if (shouldNotDecorate(error)) {
        return `${errorString}\n    at ${frames.join('\n    at ')}`;
      }

      const [
        sourceLine,
        resourceName,
        lineNumber,
        startColumn,
        endColumn,
      ] = message;

      // same output we have now
      return `${resourceName}:${lineNumber}
${sourceLine}
${' '.repeat(startColumn)}${'^'.repeat(endColumn - startColumn)}
${errorString}
    at ${frames.join('\n    at ')}`;
});

i have a patch here (https://chromium-review.googlesource.com/c/v8/v8/+/1119768) to implement the API callback in V8 (and after that lands I have another patch lined up to make v8::StackTrace movable to js-land) but before any of that happens V8 wants to know we will actually use this.

The only thing not handled here is when node has an uncaught non-error exception:

node/src/node.cc

Lines 976 to 985 in 811598b

	if (!can_set_arrow || (mode == FATAL_ERROR && !err_obj->IsNativeError())) {
	if (env->printed_error())
	return;
	Mutex::ScopedLock lock(process_mutex);
	env->set_printed_error(true);
	uv_tty_reset_mode();
	PrintErrorString("\n%s", arrow);
	return;
	}

but i think we can still make that work if we really want to (i would argue that we shouldn't)

/cc @hashseed @schuay @addaleax @Trott

[jdalton]

Is there a v8 issue related to the patch specifically? I see related issues but not one specifically for adding setPrepareStackTraceCallback. Is that a global function or one hanging off of Error?

[devsnek]

@jdalton it doesn't expose anything on the js side, it's a C++ API. I just wrote the example in js for simplicity. Also there are I think two bugs associated with that patch, they should be listed in the commit message.

[jdalton]

I just wrote the example in js for simplicity. Also there are I think two bugs associated with that patch, they should be listed in the commit message.

I saw those, I'm looking for one specifically for the new API the patch is introducing (I'm not seeing any way to provide feedback and a issue specifically for it would be a way).

Is the plan to eventually expose this API on the Error object? I'd like stack decorating to be something user-land can use too.

[devsnek]

@jdalton there's https://github.com/tc39/proposal-error-stacks and my example above leaves Error.prepareStackTrace intact. I would consider explicitly enabling userland here out of scope, as this is engine-specific.

[jdalton]

I would consider explicitly enabling userland here out of scope, as this is engine-specific.

If the goal is to remove the existing way of decorating errors then I'd ask that we consider exposing a way for users to customize decorating. This could be with a symbol much like util.inspect.custom.

[devsnek]

@jdalton i think your concerns seem better suited for tc39/proposal-error-stacks#20. the solution to your problem shouldn't be node+v8-specific.

[jdalton]

I'm not a fan of removing/changing the existing functionality without a user-land path. Does the V8 patch and intent to rework stack trace decoration require on the tc39/proposal-error-stacks to be accepted as a prerequisite?

[devsnek]

@jdalton no - this proposal changes nothing to userland except that more errors will have decorated stacks. the same APIs that you would have used (such as Error.prepareStackTrace) exist in the same way and are actually safer to use because node won't decorate your stacks.

[jdalton]

As we progress it would be rad to start thinking about possible util APIs (symbol, etc.) that developers can use to signal that their error stack is already decorated to avoid being re-decorated by Node. Currently there is a pseudo private way (that you're familiar with), but as we move away from that internally it would be great to provide a proper path for users here.

[devsnek]

As we progress it would be rad to start thinking about possible util APIs (symbol, etc.) that developers can use to signal that their error stack is already decorated to avoid being re-decorated by Node.

with this proposed change, that is fixed.

it would be great to provide a proper path for users here.

IMO the way to move with that is do the proposed System.getStack and generate the string and then just set errorInstance.stack = ..., but again i think that is out of scope of this proposal.

[jdalton]

with this proposed change, that is fixed.

Can you dig into how it's fixed a bit more. How is it decoration detected?

[devsnek]

@jdalton you don't have to detect it. With this change if Error.prepareStackTrace from userland exists node will defer to it, instead of decorating the output.

[jdalton]

Ah, I don't know if that's granular enough. Currently, user-land can mark one error as decorated and let others pass through without decoration (to be decorated by Node). If we use Error.prepareStackTrace setting as the flag there would be no way to mark one as decorated and one as not since it would assume all being prepared are decorated?

Update:

Can you provide a usage example. Maybe that would clear things up.

[devsnek]

@jdalton if you're alluding to performing decoration where we just take a stack string and modify it, i don't think its worth supporting in this case. There are plenty of cases already where an error is decorated by not-node and has no arrow symbol or an error has an arrow symbol but was not decorated because it slipped through decorateErrorStack. Additionally arrow symbols aren't a public api and i don't want to reify that.

For the moment the safe way to decorate a stack is:

const { prepareStackTrace } = Error;
Error.prepareStackTrace = (error, frames) => { ... };
myError.stack; // trigger pesudo-getter
Error.prepareStackTrace = prepareStackTrace

[jdalton]

If I break down your example you're saying that with the proposed approach Node will use the prepare callback to intercept the builtin prepareStackTrace and decorate it to taste. If the user overwrites the Error.prepareStackTrace and generates the stack (triggering the getter) then it will use the user-land Error.prepareStackTrace, avoid the builtin one (and bypass the Node callback). I see it, it's a little clunky. I would dig a way to do this in a more straight forward way.

If I hold on to the builtin prepareStackTrace could I use that within a custom one to punt to builtin (and trigger the Node callback) without restoring it on Error.prepareStackTrace?

[devsnek]

@jdalton I agree that Error.prepareStackTrace is not ideal, and that userland should have better ways of making errors their own. However again i'm trying very hard to not introduce any new semantics here or change existing ones. This is designed to be more or less invisible to userland. I don't want to step on the toes of the ecma262 stack proposal.

if you want we can run an analsys over npm and see how many people are using process.binding('util')'s hidden properties but it is terrible and i don't think its a case we need to support.

[jdalton]

I agree that Error.prepareStackTrace is not ideal, and that userland should have better ways of making errors their own.

Cool!

This is designed to be more or less invisible to userland. I don't want to step on the toes of the ecma262 stack proposal.

Just as Node allows customizing object inspection with util.inspect.custom it could allow customizing stack decorators (checking within the prepateStackTrace callback) if the error object has something like a util.errorDecorator symbol to allow customization, without stepping on toes, while making a better userland approach, and also following paths already traveled by Node for other customizations.

[devsnek]

@jdalton but as soon as we start to discuss implementation it gets hairy because i don't agree that symbols on errors is the right way to do this, even if i was interested in adding a new feature here.

We can always discuss adding things later. I really just want to discuss the changes i outlined originally, with adding new APIs being out of scope.

[jdalton]

but as soon as we start to discuss implementation it gets hairy because i don't agree that symbols on errors is the right way to do this, even if i was interested in adding a new feature here.

That's cool too. Start noodling on it. Any intent to rework Node's trace decoration should keep user-land customization in mind is all.

[mmarchini]

cc @nodejs/diagnostics as I believe this might be of our interest as well.

[hashseed]

Note that V8 currently doesn't expose a default Error.prepareStackTrace implementation. Rather, the default stack trace formatting behavior is overridden if Error.prepareStackTrace is set.