lib: ensure every unhandled promise reason is wrapped in an error

[jd-carroll]

This change adds more debugging context to any unhandled promise.
If the unhandled promise does not have a reason which is an error,
it is very difficult to determine where the error came from. This
ensures every reason has an appropriate stack trace before the
"next_tick" when the stack is lost.

Refs: #16768

If people like this change, I will happily add any tests/documentation as well.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows [commit guidelines]

[devsnek]

i would say that this should only be done if the rejection value is a string, and even then i wouldn't be enthusiastic about the coercion to an error.

[Trott]

Hi, @jd-carroll! Welcome, and thanks for the pull request! This will need a test before landing. If you want to try to write the test yourself, there is a guide in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md. It's also possible that if this feature/change gets support, someone else might offer up a test, as I imagine the test file for this would be short.

[jd-carroll]

I am certainly open to other ideas / strategies, but the intention is to capture a stacktrace when it is actually meaningful. We recently had to chase down an unhandled promise rejection which was rejecting with a null value. This was a very difficult task as --trace-warnings provided no value and essentially required us to instrument our code (including modifying things under node_modules) to an unreasonable level.

So if there is a better way to capture the stack when the rejection actually occurs, and not on the next-tick (when --trace-warnings captures the stack) 👍

[AyushG3112]

Would it make more sense to hide this under a flag? Something like --trace-unhandled-rejections

[BridgeAR]

I like the general idea about improving this. However, I wonder if this is the right approach in this case. It somewhat mixes two different concerns. I personally feel like it would be best to print a warning in general in case a promise is rejected with a primitive. Some people might want to rely those though, so it could be a bit intrusive for these people. But most people are likely not aware that there is no stack trace in case a primitive is rejected.

@benjamingr @addaleax what do you think about adding a specific option to first opt-in and as a semver-major opt-out to receive a warning with a stack trace in case a promise is rejected with a primitive?

[benjamingr]

We did this in bluebird in version 2 and removed this behaviour in bluebird 3.

People found this confusing and we ended up removing it.

What we can do however is to create the Error (and not set the reason to it) and to only log it (with the stack trace) by default in our handler without exposing it.

That said - this would require the actual stack context for the unhandled rejection - the code in this PR creates the exception way too late.

I think this might be doable when we get the async-stack-traces API from V8 - but I think it makes sense to do it after your planned logic rewrite with the setImmediate and closure replacing the WeakMap.

[addaleax]

@benjamingr I think your suggestion is already implementable – the V8 PromiseRejectMessage type has a stack_trace_ property, it just isn’t exposed at this point. And even that might be an oversight, it would be odd for V8 to use Local<> fields if they don’t intend to make it available to API users

[benjamingr]

@bmeurer can you please confirm that is something you can/want to expose?

[bmeurer]

@benjamingr @addaleax Not sure this was intended to be exposed. @hashseed and @gsathya might know.

[hashseed]

This indeed seems to be an oversight and could be added. The stack trace only exists for rejections caused by a thrown exception, and if detailed stack traces were enabled with Isolate::SetCaptureStackTraceForUncaughtExceptions, though.

[benjamingr]

Ideally Isolate::SetCaptureStackTraceForUncaughtExceptions will be the default in the future - I definitely think there is debugging value in exposing this :)

[benjamingr]

Hey, this seems to have stalled. @jd-carroll are you interested in exploring the route outlined by Anna here #21372 (comment) ?

[jd-carroll]

@benjamingr I would love to submit a PR! But i’m a little lost as to where the most appropriate fix would be. Is this something that belongs in the C layer or in Node’s integration with the V8 runtime?

Highly motivated to contribute something meaningful 👍

[benjamingr]

@hashseed @addaleax would either of you have time to guide @jd-carroll (not urgent) with those changes?

[addaleax]

@benjamingr @jd-carroll Sure! The first step would be exposing the stack_trace_ field in V8, I think. Most basic information for checking out the V8 source & contributing to it can be found in their wiki, but feel free to let us know if you need any help.

The change itself would look something along the lines of adding a sibling method to the existing ones, e.g. https://github.com/v8/v8/blob/56baf56790de439b3f69e887e94beb3b301ed77c/include/v8.h#L6669

[jasnell]

What's the status on this one?

[Trott]

Does someone want to try to get this across the finish line? Or should this be closed?

@nodejs/promises-debugging

[benjamingr]

Going to close this at the moment, feel free to continue discussion in an issue. Thanks!