errors: display Node.js version at the end of stacktraces on fatal exception that causes exit

[theoludwig]

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]

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

[theoludwig]

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]

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 :

thrownewError("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]

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]

@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]

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]

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]

@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]

@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

	}elseif(!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.
	}
	returnfalse;

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

node/src/node_errors.cc

Line 282 in ba84524

staticvoidReportFatalException(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]

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

Version should be printed

errors.js content :

thrownewError('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{thrownewError('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{getstack(){thrownewError('weird throw but ok')},getname(){thrownewError('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]

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]

@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]

@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]

i think we should definitely keep the indentation.

[mhdawson]

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]

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

[mhdawson]

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

[richardlau]

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]

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]

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

[aduh95]

test-error-reporting is failing on Windows.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[theoludwig]

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

[targos]

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]

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

[mhdawson]

Landed as 59d3d54

[mhdawson]

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