Don't destructure errors using v8::Exception::create_message(), just read error.stack

[nayeemrmn]

To fix #2703 and #2710.

We get structured error data using v8::Exception::create_message() and then format them into the colourful strings that get printed on exit. However the stack trace returned by that function is missing async frames among other issues: #2703 (comment). The stack trace we want is not stored structured-ly by V8, but is available as a string in error.stack We should instead just print error.stack, regex through it and inject colouring that way if wanted. EDIT: The structured version of this correct stack trace can be preserved by saving it from the Error.prepareStackTrace() callback.

It's pretty bad that uncaught errors from async ops print useless stack traces.

Look at the error printed by Deno.open("nonexistent"). Imagine that in the logs for a large code base.

[bartlomieju]

CC @piscisaureus I believe that's one of the issues we discussed yesterday

[cknight]

I was about to raise a similar issue. Just to add context:

await Deno.open("nonexistent") gives:

error: Uncaught NotFound: No such file or directory (os error 2)
► $deno$/dispatch_json.ts:40:11
    at DenoError ($deno$/errors.ts:20:5)
    at unwrapResponse ($deno$/dispatch_json.ts:40:11)
    at sendAsync ($deno$/dispatch_json.ts:91:10)

Neither the function open() nor where it was called from appear in the stacktrace.

[nayeemrmn]

@ry What do you think of this solution? There doesn't seem to be any other without changes to V8. Shouldn't it be a goal to fix #2703 before 1.0?

[ry]

@nayeemrmn It's quite beneficial to have structured exception in Rust, so I would like to find a way to preserve that. Have you tried --v8-flags=--async-stack-traces? Do we have a simple test case that demonstrates exceptions with async ops?

[nayeemrmn]

@ry We've gone through that. A test case is given in #2703 and you tried that flag in #2709. There is no way. As explained in #2703 (comment): there are two stack traces, the one we currently get is unconditionally bad and the other is only stored as a string in error.stack. Maybe this should be brought up with V8 people?

[ry]

@nayeemrmn Hah sorry. There's too much stuff going on - I forget the details : )

I will discuss it with @piscisaureus and see if we can find a solution.

[piscisaureus]

@nayeemrmn Is the information you need provided to the Error.prepareStackTrace callback?

[nayeemrmn]

@piscisaureus Ah, it is! That's the structured version of the correct stack trace that is discarded from error.stack.

I guess in our Error.prepareStackTrace() definition, we should save it somewhere it can be accessed from JSError::from_v8_exception() and used in place of v8::Exception::create_message(). Would that be right?

[kevinkassimo]

@nayeemrmn or in a raw form, I can save this error in some place that is accessible later from JS (e.g. window) during check_promise_exceptions and fire a console.log with formatError on it when exiting (just did a hack on it and the async stack traces are all kept)

[piscisaureus]

I guess in our Error.prepareStackTrace() definition, we should save it somewhere it can be accessed from JSError::from_v8_exception() and used in place of v8::Exception::create_message(). Would that be right?

Sounds good to me.

@nayeemrmn or in a raw form, I can save this error in some place that is accessible later from JS (e.g. window)

Sounds good to me too.

during check_promise_exceptions and fire a console.log with formatError on it when exiting (just did a hack on it and the async stack traces are all kept)

check_promise_exceptions() would not be the right place for this; it deals only with unhandled promise exceptions. The right place to pick up any stashed-away async stack frames would be in JSError::from_v8_exception().

[kevinkassimo]

@piscisaureus But isn't the intention of this to deal with unhandled promise exceptions? Any caught TLA errors would be handled by users anyways and go through prepareStackTrace.

But yeah it is definitely better if we are able do it just in from_v8_exception since it might cover some other scenarios if they exist. check_promise_exception is still a valid fallback though.

[piscisaureus]

@piscisaureus But isn't the intention of this to deal with unhandled promise exceptions? Any caught TLA errors would be handled by users anyways and go through prepareStackTrace.

Sure enough, but only unhandled exceptions, or any exception (including the ones caught and logged or rethrown)? Maybe I don't understand the problem well enough; if you have a (partial) solution, let's open a PR and discuss that.

[nayeemrmn]

@kevinkassimo The point is that our system in Rust for formatting uncaught errors reads some weird forgotten debug stack trace which is populated differently than the one in error.stack. Nothing specifically to do with promises, not capturing async frames is just one issue. We just need to read the correct stack trace when constructing a JSError.

[kevinkassimo]

I hacked up a demo at #4232 (maybe we could move actual prop retrieval to JS side instead of pushing the actual callsite objects)