Stalled top-level promise detection for TLA

[zuozp8]

Version

v18.0.0

Subsystem

modules

What steps will reproduce the bug?

create file 1.mjs with following content

await new Promise(() => {})

run it and check code returned by process with node 1.mjs && echo $?

What is the expected behavior?

I would like to be verbousely warned that script didn't reach it's end

What do you see instead?

process finished with error code 13, with nothing in console

> node 1.mjs && echo $?
13

[RaisinTen]

Isn't this expected behavior?

node/lib/internal/modules/esm/handle_process_exit.js

Lines 3 to 8 in 2691222

	// Handle a Promise from running code that potentially does Top-Level Await.
	// In that case, it makes sense to set the exit code to a specific non-zero
	// value if the main code never finishes running.
	function handleProcessExit() {
	process.exitCode ??= 13;
	}

[zuozp8]

You're right, it seems that node recognizes that there are no pending handlers and closes, i got the same result from code

await new Promise(r => setTimeout(r, 100).unref())

for me return code 13 (that usually means ACCESS_DENIED in linux) was suspicious, I expected it to come from kernel

Printing there a warning (like we had with UnhandledPromiseRejectionWarning) would be nice

[targos]

All possible Node.js exit codes are documented in https://nodejs.org/api/process.html#exit-codes
It seems they are just assigned sequentially.

[RaisinTen]

Printing there a warning (like we had with UnhandledPromiseRejectionWarning) would be nice

Would you like to send a PR to implement that?

@nodejs/loaders does this sound like a good idea?

[JakobJingleheimer]

Sorry, how is this related to loaders? Is it happening inside a custom loader? I don't see anything here to suggest it is—but if it is, I'm wondering if this is something I might catch in #42623.

[RaisinTen]

I'm sorry, I meant to ping @nodejs/modules because of

node/doc/contributing/collaborator-guide.md

Line 822 in 4f9bc41

| `src/module_wrap.*`, `lib/internal/modules/*`, `lib/internal/vm/module.js` | @nodejs/modules |

. Please ignore my previous ping.

[zuozp8]

I can prepare PR after modules team approves the idea

[devsnek]

you might need to revive this change in v8: https://chromium-review.googlesource.com/c/v8/v8/+/2341765

[guybedford]

@devsnek what would it take to revive that? @mhdawson @jasnell does Node.js have a process for directing development work on V8 where it does not directly align with the needs of Chrome? There's a few modules things like this, and this is a great example of differing needs of V8 for Chrome and the needs of V8 for Node.js (like modules GC and other hooks).

[devsnek]

fwiw they were not against that change, i just lost track of working on it. i'm sure they'd be happy if someone were to CL an updated version.

[mhdawson]

@guybedford I don't think we have a process. In the past I think individuals like @joyeecheung and possible @legendecas have submitted PRs and stickhandled them through the V8 process. @devsnek mentions generally V8 has accepted changes.

[guybedford]

I guess the best we can do is clearly mark the direction here then. I've updated the title and added the confirmed bug label, and further suggestions welcome as well.

Top-level await stalled promise handling should be supported by V8. The V8 solution to this problem has already been worked on by @devsnek in #42868 (comment), but it needs further work to land.