module: handle Top-Level Await non-fulfills better #34640
Conversation
Handle situations in which the main `Promise` from a TLA module is not fulfilled better: - When not resolving the `Promise` at all, set a non-zero exit code (unless another one has been requested explicitly) to distinguish the result from a successful completion. - When rejecting the `Promise`, always treat it like an uncaught exception. In particular, this also ensures a non-zero exit code. Refs: nodejs#34558
nodejs-github-bot
added
the
process
addaleax
added
esm
|
@addaleax if this PR lands would you feel like the questions are resolved or are there still things we need to sort out? |
|
@MylesBorins In my very subjective opinion, this PR would bring things where they need to be. ;) |
github-actions
bot
removed
the
request-ci
| @@ -2588,6 +2588,8 @@ cases: | |||
| and generally can only happen during development of Node.js itself. | |||
| * `12` **Invalid Debug Argument**: The `--inspect` and/or `--inspect-brk` | |||
| options were set, but the port number chosen was invalid or unavailable. | |||
| * `13` **Unfinished Top-Level Await**: `await` was used outside of a function | |||
There was a problem hiding this comment.
this will be used if any part of the import() process stalls, for example a loader returning a promise that never resolves.
There was a problem hiding this comment.
Are you suggesting to explicitly list that special case? I would prefer not to, if you’re working on an ESM loader then you can probably imagine that import() stalling would look like unfinished TLA. The far more common case is going to be actual TLA usage, I assume
There was a problem hiding this comment.
No I'm just saying, with the current implementation, 13 doesn't actually imply the lack of promise resolution came from a source text module with TLA. I think the best we could do is moving the exitCode check to where module.evaluate() is called, although that still isn't inherently a source text module using TLA.
There was a problem hiding this comment.
To be clear, I consider any kind of failure to finish the main module’s evaluation, whether through TLA or not, something that should not be considered a successful operation. This is intentional, and I would be against restricting the check here to only module.evaluate().
There was a problem hiding this comment.
I agree we shouldn't consider those success cases. I'm saying, the message says "Unfinished Top-Level Await" but it can be caused by other things, which is potentially confusing. It might be better to rename it to "ESM Import Stalled" or something and mention that while many things can cause it, it would probably be caused by await (promise that never resolves)
There was a problem hiding this comment.
I don’t think most people would know what “ESM Import Stalled” means.
Again, I think this is mostly relevant when you’re actually developing a loader or similar, and in that case you most likely don’t even need any explanation at all.
There was a problem hiding this comment.
IMO, Stalled Top-Level Await will be the best mix of both 🙄
github-actions
bot
removed
the
request-ci
|
CI: https://ci.nodejs.org/job/node-test-pull-request/32641/ (:green_heart:) |
addaleax
added
the
author ready
|
If no one objects I plan to land this right at the 48 hour mark so we can unblock TLA for 14.8.0 |
Handle situations in which the main `Promise` from a TLA module is not fulfilled better: - When not resolving the `Promise` at all, set a non-zero exit code (unless another one has been requested explicitly) to distinguish the result from a successful completion. - When rejecting the `Promise`, always treat it like an uncaught exception. In particular, this also ensures a non-zero exit code. Refs: #34558 PR-URL: #34640 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
|
Landed in e948ef3 |
Handle situations in which the main `Promise` from a TLA module is not fulfilled better: - When not resolving the `Promise` at all, set a non-zero exit code (unless another one has been requested explicitly) to distinguish the result from a successful completion. - When rejecting the `Promise`, always treat it like an uncaught exception. In particular, this also ensures a non-zero exit code. Refs: #34558 PR-URL: #34640 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
Handle situations in which the main `Promise` from a TLA module is not fulfilled better: - When not resolving the `Promise` at all, set a non-zero exit code (unless another one has been requested explicitly) to distinguish the result from a successful completion. - When rejecting the `Promise`, always treat it like an uncaught exception. In particular, this also ensures a non-zero exit code. Refs: #34558 PR-URL: #34640 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
Handle situations in which the main
Promisefrom a TLA moduleis not fulfilled better:
Promiseat all, set a non-zero exit code(unless another one has been requested explicitly) to distinguish
the result from a successful completion.
Promise, always treat it like an uncaughtexception. In particular, this also ensures a non-zero exit code.
Refs: #34558
I’m sorry, this is a bit late. I’ve added the dont-land label on #34558 – I remember these points being brought up before in discussion (not sure where), but it seems like they weren’t revisited before unflagging TLA on
master. I personally think they need to be addressed before we actually ship this, and in particular silently treatingawait unresolvedorawait rejectedas a success is not acceptable.@MylesBorins @nodejs/modules-active-members
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes