Skip to content
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

Implement ES stage-3 proposal: error-cause #38725

Closed
slorber opened this issue May 18, 2021 · 11 comments · Fixed by #41002
Closed

Implement ES stage-3 proposal: error-cause #38725

slorber opened this issue May 18, 2021 · 11 comments · Fixed by #41002
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. feature request Issues that request new features to be added to Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@slorber
Copy link

slorber commented May 18, 2021

Is your feature request related to a problem? Please describe.

Wrapping/enhancing errors with more context is complicated in JavaScript, while it's supported in multiple languages (like Java)

Describe the solution you'd like

Implement this stage-3 error cause proposal: https://github.com/tc39/proposal-error-cause

It's also worth making sure that logging an error with a causal chain logs the full causal chain recursively by default: this would make it easier to find the root cause of a bug without requiring the user to loop over the causal chain.

@mcollina suggested to open an issue

Describe alternatives you've considered

Using a custom non-std error library, but this would only work in my own code.

@targos
Copy link
Member

targos commented May 18, 2021

Do you mean add e.cause = something before throwing some core errors?

@slorber
Copy link
Author

slorber commented May 18, 2021

I mean implementing the formal ES proposal spec: https://github.com/tc39/proposal-error-cause

function runTheJob() {
  try {
    someRiskyComputation();
  } catch (e) {
    throw new Error("The job failed, see why: ", { cause: e });
  }
}

try {
  runTheJob();
} catch (e) {
  console.error(e);
}

On failure, it should print something like:

Error: The job failed, see why: 
    ...someStackTraceItems
Caused by: undefined is not a function
    ...someStackTraceItems

Maybe some internal NodeJS code can benefit from this in some places, but I can't tell where exactly, and the spec implementation is the first step before thinking about using this :)

To give an example, here's what a Java stack trace looks like:

Exception in thread "main" com.myproject.module.MyProjectFooBarException: The number of FooBars cannot be zero
    at com.myproject.module.MyProject.anotherMethod(MyProject.java:19)
    at com.myproject.module.MyProject.someMethod(MyProject.java:12)
    at com.myproject.module.MyProject.main(MyProject.java:8)
Caused by: java.lang.ArithmeticException: The denominator must not be zero
    at org.apache.commons.lang3.math.Fraction.getFraction(Fraction.java:143)
    at com.myproject.module.MyProject.anotherMethod(MyProject.java:17)
    ... 2 more

(source: https://www.twilio.com/blog/how-to-read-and-understand-a-java-stacktrace).

The goal is to replace annoying code such as:

try {
  someRiskyJob(jobData)
} catch (e) {
  console.error("I'm logging some message because JS errors do not have a cause and I need to provide more context, here's some useful data:" + JSON.stringify(jobData));
  throw e;
}

@targos
Copy link
Member

targos commented May 18, 2021

But the spec changes internals of the Error constructor, which is implemented by the V8 engine, not Node.js directly.

@targos
Copy link
Member

targos commented May 18, 2021

/cc @legendecas. As the author of the proposal and a Node.js collaborator, do you think there's something that we can do to have something in Node.js before V8 implements it?

@targos targos added errors Issues and PRs related to JavaScript errors originated in Node.js core. v8 engine Issues and PRs related to the V8 dependency. labels May 18, 2021
@targos
Copy link
Member

targos commented May 18, 2021

I just realized that V8 already implemented it behind a flag a month ago: https://chromium-review.googlesource.com/c/v8/v8/+/2784681

As soon as the feature is stabilized, we can look into backporting it.

@targos
Copy link
Member

targos commented May 18, 2021

Note that the proposal at https://github.com/tc39/proposal-error-cause is just about adding a cause property to the errors. It doesn't change how stack traces are generated and V8 doesn't include the cause in its stack trace:

$ ./node --harmony-error-cause
Welcome to Node.js v17.0.0-pre.
Type ".help" for more information.
> new Error('test', { cause: new Error('other') })
Error: test
    at REPL1:1:1
    at Script.runInThisContext (node:vm:131:12)
    at REPLServer.defaultEval (node:repl:524:29)
    at bound (node:domain:416:15)
    at REPLServer.runBound [as eval] (node:domain:427:12)
    at REPLServer.onLine (node:repl:846:10)
    at REPLServer.emit (node:events:377:35)
    at REPLServer.emit (node:domain:470:12)
    at REPLServer.Interface._onLine (node:readline:452:10)
    at REPLServer.Interface._line (node:readline:797:8)
> new Error('test', { cause: new Error('other') }).cause
Error: other
    at REPL2:1:28
    at Script.runInThisContext (node:vm:131:12)
    at REPLServer.defaultEval (node:repl:524:29)
    at bound (node:domain:416:15)
    at REPLServer.runBound [as eval] (node:domain:427:12)
    at REPLServer.onLine (node:repl:846:10)
    at REPLServer.emit (node:events:377:35)
    at REPLServer.emit (node:domain:470:12)
    at REPLServer.Interface._onLine (node:readline:452:10)
    at REPLServer.Interface._line (node:readline:797:8)
> new Error('test', { cause: new Error('other') }).stack
'Error: test\n' +
  '    at REPL3:1:1\n' +
  '    at Script.runInThisContext (node:vm:131:12)\n' +
  '    at REPLServer.defaultEval (node:repl:524:29)\n' +
  '    at bound (node:domain:416:15)\n' +
  '    at REPLServer.runBound [as eval] (node:domain:427:12)\n' +
  '    at REPLServer.onLine (node:repl:846:10)\n' +
  '    at REPLServer.emit (node:events:377:35)\n' +
  '    at REPLServer.emit (node:domain:470:12)\n' +
  '    at REPLServer.Interface._onLine (node:readline:452:10)\n' +
  '    at REPLServer.Interface._line (node:readline:797:8)'
>

@slorber
Copy link
Author

slorber commented May 19, 2021

Edit: looks like error logging is not std, cf the Firefox issue:


I have no idea how engines decide to log console errors (and if there is any std error logging), but this looks not related to the JS language, so not too surprised it's not part of the TC39 spec. Any idea?

The proposal shows:

try {
  await doJob();
} catch (e) {
  console.log(e);
  console.log('Caused by', e.cause);
}

However I really hope in the long run we won't need to have to log the cause, particularly when this is potentially a recursive structurer

console.log('Caused by', e.cause.cause.cause.cause);

Having this supported in ES looks like a good first step in that direction

@devsnek
Copy link
Member

devsnek commented May 19, 2021

@targos yeah we need updates for both error causes and aggregate errors in inspect 😔

@hemanth
Copy link
Contributor

hemanth commented May 19, 2021

We had presented the below for stage-3 and hoping other engines will do the same as FF!

image

AFAIR current V8 implementation doesn't show cause in the stacktrace.

@legendecas
Copy link
Member

legendecas commented May 20, 2021

The spec of error cause is meant for language implementations but for runtime nor devtools. So the things we can do in Node.js are:

  1. Pretty print the cause property in console/util.inspect etc.
  2. Decorate Node.js internal errors to chain with the cause property.

I believe we can start with item 1 as whether or not the error constructor can receive the new parameter, the cause property on error objects can always be inspected with or without v8's change.

Item 2 requires v8's change on error constructor so that we can create new errors with cause property. But that's not necessary requirement: we can still assign the cause property after the constructor either ways.

Devtools have their own protocols, like @hemanth mentioned. Currently the chrome devtools protocol doesn't reflect error object's cause by default. Anyway, that's up to v8 and chrome devtools, there is nothing Node.js can do as far as I could tell.

@DerekNonGeneric DerekNonGeneric added the feature request Issues that request new features to be added to Node.js. label May 22, 2021
@voxpelli
Copy link

Error causes was released in 16.9.0 as a consequence of the upgrade to V8 9.3: https://nodejs.org/en/blog/release/v16.9.0/#error-cause

  1. Pretty print the cause property in console/util.inspect etc.

Looks like the inner workings of console.log uses the formatWithOptions from lib/internal/util/inspect.js.

return ReflectApply(formatWithOptions, null, args);

function formatWithOptions(inspectOptions, ...args) {

And formatWithOptions uses its own formatError which picks the stack, or if no stack, then the toString() of Error:

function formatError(err, constructor, tag, ctx, keys) {
const name = err.name != null ? String(err.name) : 'Error';
let len = name.length;
let stack = err.stack ? String(err.stack) : ErrorPrototypeToString(err);

Lastly it does some tweaking of the output, and that's probably where the causes could be iterated through and appended, probably by recursively calling formatError on the causes until either there are no more causes, or an already formatted error is encountered (preventing infinite circular cause recursion)

I can try and make a stab at doing that if it sounds like a good idea to everyone?

  1. Decorate Node.js internal errors to chain with the cause property.

I tried searching through the Node.js projects for cases where this could be useful to do, my regexps returned no cases, do you know of any @legendecas?

nodejs-github-bot pushed a commit that referenced this issue Dec 2, 2021
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41002
Fixes: #40859
Fixes: #38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Dec 14, 2021
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41002
Fixes: #40859
Fixes: #38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41002
Fixes: #40859
Fixes: #38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41002
Fixes: #40859
Fixes: #38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#41002
Fixes: nodejs#40859
Fixes: nodejs#38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
While inspecting errors, always visualize the cause. That property
is non-enumerable by default while being useful in general for
debugging.

Duplicated stack frames are hidden.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41002
Fixes: #40859
Fixes: #38725
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. feature request Issues that request new features to be added to Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants