errors: display Node.js version at the end of stacktraces on fatal exception that causes exit by theoludwig · Pull Request #38332 · nodejs/node

theoludwig · 2021-04-21T15:37:25Z

Display Node.js version at the end of stacktraces
on fatal exception that causes exit.
Easier for debugging so you don't have
to ask "what node version are you on?",
it is directly in the error the user
copy/paste from when asking for help.

Fixes: #29731

BridgeAR · 2021-04-22T17:48:24Z

If we want to add the Node.js version, I would only do that to fatal exceptions that cause Node.js to end.

theoludwig · 2021-04-22T18:09:38Z

If we want to add the Node.js version, I would only do that to fatal exceptions that cause Node.js to end.

Right, thanks for sharing your thoughts.
I don't really understand what you mean, because every errors exit the program (if don't try/catch them of course).

theoludwig · 2021-04-27T09:39:26Z

Before continuing further the work on this PR, I think it's better to gather some feedbacks from the Node.js community and main contributors.

I made a file called app.js with this inside :

throw new Error("Some random error...")

Currently (with Node.js v16) by doing node app.js, I've got this output to stderr :

/home/divlo/Documents/testing/app.js:1
throw new Error("Some random error...")
^

Error: Some random error...
    at Object.<anonymous> (/home/divlo/Documents/testing/app.js:1:7)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Function.Module._load (node:internal/modules/cjs/loader:828:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47

With the changes I made on this PR, here's the new output from the same file:

/home/divlo/Documents/testing/app.js:1
throw new Error("Some random error...")
^

Error: Some random error...
    at Object.<anonymous> (/home/divlo/Documents/testing/app.js:1:7)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Function.Module._load (node:internal/modules/cjs/loader:828:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47
    via Node.js v17.0.0-pre

I see that's also bumping the major version number, is that considered as a BREAKING CHANGE or is it only when testing locally with the local build ?

Any thoughts ?
Have you something to add @ORESoftware ?

aduh95 · 2021-04-27T10:10:50Z

I see that's also bumping the major version number

That's not bumping the major version number, the master branch is at version v17.0.0-pre so what you are seeing is expected. If you revert your commit and run make -j12 && out/Release/node --version, you'll see you're still at v17.0.0-pre.

is that considered as a BREAKING CHANGE

node/doc/guides/collaborator-guide.md

Lines 341 to 348 in 005ebaf

    
           Examples of breaking changes include: 
        
           * Removal or redefinition of existing API arguments. 
        
           * Changing return values. 
        
           * Removing or modifying existing properties on an options argument. 
        
           * Adding or removing errors. 
        
           * Altering expected timing of an event. 
        
           * Changing the side effects of using a particular API.

I don't think it qualifies as breaking change.

If we want to add the Node.js version, I would only do that to fatal exceptions that cause Node.js to end.

Right, thanks for sharing your thoughts.
I don't really understand what you mean, because every errors exit the program (if don't try/catch them of course).

I think they meant that doing console.log(new Error().stack) should not out the Node.js version, it should only be added when the error is making the process crash (so when it's been thrown and not caught).

mhdawson · 2021-05-18T14:55:37Z

@gireeshpunathil what's your thought on this one. I'm thinking it might be useful to have the additional info in the strack trace but I'm wondering if there are some down sides that I'm not thinking of?

theoludwig · 2021-05-22T15:52:09Z

Thanks for your concise answer! @aduh95

Right @mhdawson, I'm happy to continue the work for this PR.
Could I have some help to fix the tests errors ?
I already tried to edit some tests to see what's going on but there is much more to fix, any insights, tips ?
Also, I make this PR ready for review, so the GitHub Actions are not skipped, so we can see what tests are failing.
Concerning the commit message, I'll follow the Node.js commit convention when everything will be good to go and green (I will reset --soft and force push), until then, I'll just commit, it makes life easier.

ORESoftware · 2021-05-23T02:58:32Z

via Node.js version at

would be a nice touch, but I doubt node runtime has that info by default...maybe its process.argv[0]

gireeshpunathil · 2021-05-23T07:28:39Z

@mhdawson , @divlo -

any extra bit of info is definitely useful from the diagnostics point of view. The side effect of this I can think of are CI and external tools - that may have static assumptions about the shape /structure of the stack trace. I suggest to run CITGM tests too, once the CI passes - that can potentially give us hint around what can break and how.

fixing CI is easy, but bit laborious.

go to each failure
find the matching test case
find what the expected value is
modify it based on the change that we made (mostly the expected is going to be a regexp, that needs to cater to the change too)

BridgeAR · 2021-05-23T13:00:56Z

@divlo if you change this to only be attached for fatal exceptions, you won't have to change as many tests. The last JS code that is run before exiting is:

node/lib/internal/process/execution.js

Lines 170 to 182 in ba84524

    
           } else if (!process.emit('uncaughtException', er, type)) { 
        
             // If someone handled it, then great. Otherwise, die in C++ land 
        
             // since that means that we'll exit the process, emit the 'exit' event. 
        
             try { 
        
               if (!process._exiting) { 
        
                 process._exiting = true; 
        
                 process.exitCode = 1; 
        
                 process.emit('exit', 1); 
        
               } 
        
             } catch { 
        
               // Nothing to be done about it at this point. 
        
             } 
        
             return false;

The stack trace itself is printed in the C++ layer:

node/src/node_errors.cc

Line 282 in ba84524

static void ReportFatalException(Environment* env,

It should look similar to:

node/src/node_errors.cc

Lines 396 to 399 in ba84524

    
           FPrintF(stderr, 
        
                   "(Use `%s --trace-uncaught ...` to show where the exception " 
        
                   "was thrown)\n", 
        
                   fs::Basename(argv0, ".exe"));

theoludwig · 2021-05-24T16:28:36Z

Following @BridgeAR recomendations, here's the new behavior (in a file called errors.js) :

Version should be printed

errors.js content :

throw new Error('Some random error...')

node errors.js output :

/home/divlo/Documents/testing/errors.js:1
throw new Error('Some random error...')
^

Error: Some random error...
    at Object.<anonymous> (/home/divlo/Documents/testing/errors.js:1:7)
    at Module._compile (node:internal/modules/cjs/loader:1109:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47
    via Node.js v17.0.0-pre

Version should not be printed because the error is in a try/catch

errors.js content :

try {
  throw new Error('Some random error...')
} catch (error) {
  console.error(error)
}

node errors.js output :

Error: Some random error...
    at Object.<anonymous> (/home/divlo/Documents/testing/errors.js:4:9)
    at Module._compile (node:internal/modules/cjs/loader:1109:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47

Version should be printed (in addition to --trace-uncaught warning)

errors.js content :

throw {
  get stack() {
    throw new Error('weird throw but ok')
  },
  get name() {
    throw new Error('weird throw but ok')
  }
}

node errors.js output :

/home/divlo/Documents/testing/errors.js:9
throw {
^
[object Object]
(Use `node --trace-uncaught ...` to show where the exception was thrown)
    via Node.js v17.0.0-pre

Also good news, the CI now pass, everything should be green. 🟢
I'm definitely up to run CITGM tests, to see if there is some breakage.

theoludwig · 2021-06-07T05:48:26Z

Now that the CI pass, what is needed to move this PR forward ? @mhdawson @gireeshpunathil

aduh95

Overall this LGTM, thanks for the hard work of fixing every stacktrace in the message tests. I'm a bit unsure if "via" is the correct word to use, but I also don't have a better suggestion :)
Just a few nits:

BridgeAR · 2021-06-07T12:00:18Z

@aduh95 I agree about the wording. What do you think about removing it including the indentation? Just the plain Node.js vX.Y.Z?

theoludwig · 2021-06-07T12:09:07Z

@aduh95 I agree about the wording. What do you think about removing it including the indentation? Just the plain Node.js vX.Y.Z?

I don't mind about the wording, both looks good to me. 😄
If I understand your feedback the error should be printed something like that :

/home/divlo/Documents/testing/app.js:1
throw new Error("Some random error...")
^

Error: Some random error...
    at Object.<anonymous> (/home/divlo/Documents/testing/app.js:1:7)
    at Module._compile (node:internal/modules/cjs/loader:1108:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Function.Module._load (node:internal/modules/cjs/loader:828:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47
Node.js v17.0.0-pre

Is it correct ?

EDIT: Sorry I closed the PR accidentally, instead of sending this message, it closed the PR. 😅.

mhdawson

LGTM

devsnek · 2021-06-07T17:55:31Z

i think we should definitely keep the indentation.

mhdawson · 2021-06-07T17:55:31Z

While I agree this should be SemVer minor I wonder if it would be prudent to avoid backporting out of an abundance of caution as it does affect a good number of our own tests and may do the same for others as well. @nodejs/tsc any thoughts on that?

targos · 2021-06-07T18:03:30Z

If you think it shouldn't be backported, then it should be semver-major.

mhdawson · 2021-06-07T18:05:32Z

I'd be ok with marking it SemVer major even though technically its may not be.

richardlau · 2021-06-07T18:23:21Z

While declaring something semver-major automatically excludes it from being backported, we can alternatively add the dont-land-on labels to achieve the same thing for things that are not semver-major but we still do not want to land on earlier release lines.

targos · 2021-06-07T18:42:37Z

While declaring something semver-major automatically excludes it from being backported, we can alternatively add the dont-land-on labels to achieve the same thing for things that are not semver-major but we still do not want to land on earlier release lines.

Sure, but if the reason for avoiding a backport is because we think the change can break someone, that's exactly what semver-major is for.

nodejs-github-bot · 2021-09-02T10:00:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39742/

aduh95 · 2021-09-02T13:36:25Z

test-error-reporting is failing on Windows.

Co-authored-by: Antoine du Hamel <[email protected]>

nodejs-github-bot · 2021-09-03T16:03:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39757/

nodejs-github-bot · 2021-09-03T16:47:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39759/

nodejs-github-bot · 2021-09-03T17:16:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39762/

nodejs-github-bot · 2021-09-03T17:32:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39764/

nodejs-github-bot · 2021-09-05T10:19:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39787/

nodejs-github-bot · 2021-09-05T11:09:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39789/

nodejs-github-bot · 2021-09-05T11:11:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39790/

theoludwig · 2021-09-05T11:39:36Z

The CI keeps failing.
Is there is anything needed to make it pass or it is not on the side of this PR?

targos · 2021-09-05T11:41:22Z

It fails because of this error (I think):

13:23:03 ERROR: The process with PID 35016 (child process of PID 42664) could not be terminated.

/cc @nodejs/build-infra

nodejs-github-bot · 2021-09-06T11:10:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39821/

ghost · 2021-09-08T17:36:45Z

Landed as 59d3d54

ghost · 2021-09-08T17:37:05Z

@divlo thanks for you work and patience on this one.

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originating in Node.js core. needs-ci PRs that need a full CI run. labels Apr 21, 2021

theoludwig mentioned this pull request Apr 21, 2021

display node.js version (process.version) at the end of stacktraces #29731

Closed

theoludwig marked this pull request as draft April 21, 2021 18:36

targos added the needs-citgm PRs that need a CITGM CI run. label May 18, 2021

theoludwig marked this pull request as ready for review May 22, 2021 15:52

aduh95 approved these changes Jun 7, 2021

View reviewed changes

Comment thread test/message/vm_dont_display_runtime_error.out Outdated

Comment thread test/parallel/test-error-reporting.js Outdated

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 7, 2021

theoludwig closed this Jun 7, 2021

theoludwig reopened this Jun 7, 2021

mhdawson approved these changes Jun 7, 2021

View reviewed changes

aduh95 removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 2, 2021

aduh95 reviewed Sep 3, 2021

View reviewed changes

Comment thread test/parallel/test-error-reporting.js Outdated

fixup!: errors: update test/parallel/test-error-reporting.js

4725e66

Co-authored-by: Antoine du Hamel <[email protected]>

aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 3, 2021

mhdawson mentioned this pull request Sep 7, 2021

Node.js Technical Steering Committee (TSC) Meeting 2021-09-09 nodejs/TSC#1082

Closed

mhdawson closed this Sep 8, 2021

theoludwig deleted the errors/display-node-version branch September 8, 2021 19:03

BethGriggs mentioned this pull request Sep 15, 2021

2021-10-19, Version 17.0.0 (Current) #40119

Merged

2 tasks

theoludwig mentioned this pull request Oct 19, 2021

errors: display Deno version at the end of stacktraces on fatal exception that causes exit denoland/deno#12498

Closed

Uh oh!

Conversation

theoludwig commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 22, 2021

Uh oh!

theoludwig commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theoludwig commented Apr 27, 2021

Uh oh!

aduh95 commented Apr 27, 2021

Uh oh!

mhdawson commented May 18, 2021

Uh oh!

theoludwig commented May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ORESoftware commented May 23, 2021

Uh oh!

gireeshpunathil commented May 23, 2021

Uh oh!

BridgeAR commented May 23, 2021

Uh oh!

theoludwig commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theoludwig commented Jun 7, 2021

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Jun 7, 2021

Uh oh!

theoludwig commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

devsnek commented Jun 7, 2021

Uh oh!

mhdawson commented Jun 7, 2021

Uh oh!

targos commented Jun 7, 2021

Uh oh!

mhdawson commented Jun 7, 2021

Uh oh!

richardlau commented Jun 7, 2021

Uh oh!

targos commented Jun 7, 2021

Uh oh!

nodejs-github-bot commented Sep 2, 2021

Uh oh!

aduh95 commented Sep 2, 2021

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 3, 2021

Uh oh!

nodejs-github-bot commented Sep 3, 2021

Uh oh!

nodejs-github-bot commented Sep 3, 2021

Uh oh!

nodejs-github-bot commented Sep 3, 2021

Uh oh!

nodejs-github-bot commented Sep 5, 2021

Uh oh!

nodejs-github-bot commented Sep 5, 2021

Uh oh!

nodejs-github-bot commented Sep 5, 2021

Uh oh!

theoludwig commented Sep 5, 2021

Uh oh!

targos commented Sep 5, 2021

Uh oh!

nodejs-github-bot commented Sep 6, 2021

Uh oh!

theoludwig commented Apr 21, 2021 •

edited

Loading

theoludwig commented Apr 22, 2021 •

edited

Loading

theoludwig commented May 22, 2021 •

edited

Loading

theoludwig commented May 24, 2021 •

edited

Loading

theoludwig commented Jun 7, 2021 •

edited

Loading