src: fix process exit listeners not receiving unsettled tla codes

[dario-piotrowicz]

fix listeners registered via process.on('exit', ... not receiving error code 13 when an unsettled top-level-await is encountered in the code

Fixes: #53551

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.14%. Comparing base (d1f8ccb) to head (7a48cfd).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56872   +/-   ##
=======================================
  Coverage   89.13%   89.14%           
=======================================
  Files         665      665           
  Lines      193167   193190   +23     
  Branches    37190    37191    +1     
=======================================
+ Hits       172185   172210   +25     
- Misses      13731    13738    +7     
+ Partials     7251     7242    -9     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/node.js	99.58% <100.00%> (+<0.01%)	⬆️
src/api/embed_helpers.cc	74.61% <100.00%> (-0.02%)	⬇️
src/api/hooks.cc	84.55% <100.00%> (+0.83%)	⬆️
src/env-inl.h	96.97% <100.00%> (+0.02%)	⬆️
src/env.h	98.14% <ø> (ø)

... and 25 files with indirect coverage changes

[joyeecheung]

Thanks for the PR. Some comments about the approaches taken.

[joyeecheung]

I think prior to emitting the event, it should also set the kHasExitCode and kExitCode fields in the exit info buffer (basically needing an implementation of Environment::set_exit_code() that is the counter part of Environment::exit_code) so that in the handler, process.exitCode would be 13, to be consistent with other paths. Also in any case, the documentation of the exit event in doc/api/process.md should also be updated to note this extra source of exit code.

[dario-piotrowicz]

Hey @joyeecheung 👋

I've been trying to implement what you suggested and I created this set_exit_code method:

does it look ok? 🙏

The issue is that it's not fully working 😖

The problem is that as I get undefined in my test:

But I am sure that env->set_exit_code() is working since if I use it when process already has an exitCode then I can actually see the exit code changing, the issue is that when there is no exit code already set this doesn't seem to set it 😕 (this makes me suspect that I'm doing something wrong with kHasExitCode... but I don't think I am?)

do you have any idea what might be going wrong? 🙏

[joyeecheung]

Oh I mean you'll need to set the exit code when it's updated to ExitCode::kUnsettledTopLevelAwait before you invoke process.emit, implementing that helper alone would get it called.

[dario-piotrowicz]

Yes, of course, I am doing that:

this should be enough, right?

[joyeecheung]

The env->set_exit_code() should only be necessary in the branch where you reassign exit_code to ExitCode::kUnsettledTopLevelAwait, since it's otherwise unchanged.

[dario-piotrowicz]

ok good point, but either way it's not working 😓

I've been trying to debug this but I could not yet figure our why process.exitCode is not getting set by this 😓

[dario-piotrowicz]

oof... I think I've missed something really obvious! 🤦

updating the exit_info_ values is not sufficient, since the getter for process.exitCode is this:

node/lib/internal/bootstrap/node.js

Lines 110 to 112 in d1f8ccb

	get() {
	return exitCode;
	},

which relies on this local variable:

node/lib/internal/bootstrap/node.js

Line 107 in d1f8ccb

let exitCode;

which is set in the setter and which is unrelated to exit_info_ 😕

right?

[dario-piotrowicz]

@joyeecheung it took me ages to solve this super silly thing, but I think I got it 😅 (I was so focused on the c++ side of things that missed to look deeper in the lib js code 😓)

I also updated the docs as you very sensibly suggested

Please have another look when you've got a sec 🙂 🙏