Stalled top-level promise detection for TLA #42868

zuozp8 · 2022-04-25T15:45:32Z

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

The text was updated successfully, but these errors were encountered:

RaisinTen · 2022-04-25T16:07:53Z

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 · 2022-04-25T16:43:05Z

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 · 2022-04-25T18:20:48Z

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 · 2022-04-26T04:23:43Z

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 · 2022-04-26T06:18:59Z

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 · 2022-04-26T06:36:27Z

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 · 2022-04-26T06:57:10Z

I can prepare PR after modules team approves the idea

devsnek · 2022-04-26T13:31:58Z

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

guybedford · 2022-04-29T13:16:03Z

@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 · 2022-04-29T13:31:39Z

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 · 2022-04-29T18:13:37Z

@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 · 2022-04-29T18:33:22Z

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.

bnoordhuis · 2022-05-05T01:28:03Z

Changes to V8 are not necessary per se, the pending promise can be found with the inspector's Runtime.queryObjects RPC method. I did a proof of concept in 2019: https://gist.github.com/bnoordhuis/e4f935a0c81c477533c9bdc54b22e837

joyeecheung · 2022-05-05T04:51:47Z

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).

I think the general convention is that once we confirm a V8 change is necessary (e.g. can reproduce with a V8-only test) we open a bug in the V8 issue tracker and maybe ping someone who's the owner of related V8 components. V8's issue tracker has a NodeJS-Hotlist label for Node.js-related requests. Then someone may take that bug from there.

joyeecheung · 2022-05-05T04:57:18Z

Also about the bug here specifically - I can try to take a look at the bug and the CL and see if I can revive it, as @devsnek suggested. (From a quick glance I think having a V8 API for this case has its merits, as the inspector isn't always available in all of our builds)

joyeecheung · 2022-06-02T08:52:45Z

FYI I have modified the v8 patch a bit and re-uploaded it in the original CL - https://chromium-review.googlesource.com/c/v8/v8/+/2341765/ may need to take some time to take care of the dynamic import use cases mentioned in the previous reviews though.

joyeecheung · 2024-03-02T05:29:04Z

I have a prototype here which prints this for the repro in the OP

Error: Detected unfinished top-level await at file:///Users/joyee/projects/node/unfinished.mjs:1
await new Promise(() => {});
^

For imported unfinished TLA:

Error: Detected unfinished top-level await at file:///Users/joyee/projects/node/mod.mjs:1
await new Promise(() => { 'mod' });
^

(By the time we exit and check for the unfinished TLA, the stack is already lost. I guess it's better than printing nothing at all).

PR-URL: #51999 Fixes: #42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

When the entry point is a module and the graph it imports still contains unsettled top-level await when the Node.js instance finishes the event loop, search from the entry point module for unsettled top-level await and print their location. To avoid unnecessary overhead, we register a promise that only gets settled when the entry point graph evaluation returns from await, and only search the module graph if it's still unsettled by the time the instance is exiting. This patch only handles this for entry point modules. Other kinds of modules are more complicated so will be left for the future. Drive-by: update the terminology "unfinished promise" to the more correct one "unsettled promise" in the codebase. PR-URL: #51999 Fixes: #42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Split the `internal/process/esm_loader` file which contains the singleton cascaded loader: - The the singleton cascaded loader now directly resides in `internal/modules/esm/loader`, where the constructor also lives. This file is the root of most circular dependency of ESM code, (because components of the loader need the singleton itself), so this makes the dependency more obvious. Added comments about loading it lazily to avoid circular dependency. - The getter to the cascaded loader is also turned into a method to make the side effect explicit. - The sequence of `loadESM()` and `handleMainPromise` is now merged together into `runEntryPointWithESMLoader()` in `internal/modules/run_main` because this is intended to run entry points with the ESM loader and not just any module. - Documents how top-level await is handled. PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

When the entry point is a module and the graph it imports still contains unsettled top-level await when the Node.js instance finishes the event loop, search from the entry point module for unsettled top-level await and print their location. To avoid unnecessary overhead, we register a promise that only gets settled when the entry point graph evaluation returns from await, and only search the module graph if it's still unsettled by the time the instance is exiting. This patch only handles this for entry point modules. Other kinds of modules are more complicated so will be left for the future. Drive-by: update the terminology "unfinished promise" to the more correct one "unsettled promise" in the codebase. PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Split the `internal/process/esm_loader` file which contains the singleton cascaded loader: - The the singleton cascaded loader now directly resides in `internal/modules/esm/loader`, where the constructor also lives. This file is the root of most circular dependency of ESM code, (because components of the loader need the singleton itself), so this makes the dependency more obvious. Added comments about loading it lazily to avoid circular dependency. - The getter to the cascaded loader is also turned into a method to make the side effect explicit. - The sequence of `loadESM()` and `handleMainPromise` is now merged together into `runEntryPointWithESMLoader()` in `internal/modules/run_main` because this is intended to run entry points with the ESM loader and not just any module. - Documents how top-level await is handled. PR-URL: #51999 Fixes: #42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: #51999 Fixes: #42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Split the `internal/process/esm_loader` file which contains the singleton cascaded loader: - The the singleton cascaded loader now directly resides in `internal/modules/esm/loader`, where the constructor also lives. This file is the root of most circular dependency of ESM code, (because components of the loader need the singleton itself), so this makes the dependency more obvious. Added comments about loading it lazily to avoid circular dependency. - The getter to the cascaded loader is also turned into a method to make the side effect explicit. - The sequence of `loadESM()` and `handleMainPromise` is now merged together into `runEntryPointWithESMLoader()` in `internal/modules/run_main` because this is intended to run entry points with the ESM loader and not just any module. - Documents how top-level await is handled. PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

When the entry point is a module and the graph it imports still contains unsettled top-level await when the Node.js instance finishes the event loop, search from the entry point module for unsettled top-level await and print their location. To avoid unnecessary overhead, we register a promise that only gets settled when the entry point graph evaluation returns from await, and only search the module graph if it's still unsettled by the time the instance is exiting. This patch only handles this for entry point modules. Other kinds of modules are more complicated so will be left for the future. Drive-by: update the terminology "unfinished promise" to the more correct one "unsettled promise" in the codebase. PR-URL: nodejs#51999 Fixes: nodejs#42868 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

zuozp8 changed the title ~~crash with error code 13 when deadlocked~~ deadlock in .mjs file causes node to exit without warning Apr 26, 2022

guybedford added the confirmed-bug Issues with confirmed bugs. label Apr 29, 2022

guybedford changed the title ~~deadlock in .mjs file causes node to exit without warning~~ Stalled top-level promise detection for TLA Apr 29, 2022

aduh95 mentioned this issue May 21, 2022

Awaiting unresolvable promise should throw instead of Node terminating the process. #43162

Closed

F3n67u mentioned this issue Jun 24, 2022

Error message for when there code evaluation is still pending and there are no pending async operations #43562

Closed

joyeecheung mentioned this issue Mar 7, 2024

module: print location of unsettled top-level await in entry points #51999

Closed

joyeecheung closed this as completed in 075c95f Mar 10, 2024

Jean-Luc-Picard-2021 mentioned this issue Dec 19, 2024

IIAFE broken for single files? #56320

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stalled top-level promise detection for TLA #42868

Stalled top-level promise detection for TLA #42868

zuozp8 commented Apr 25, 2022 •

edited

Loading

RaisinTen commented Apr 25, 2022

zuozp8 commented Apr 25, 2022 •

edited

Loading

targos commented Apr 25, 2022

RaisinTen commented Apr 26, 2022

JakobJingleheimer commented Apr 26, 2022

RaisinTen commented Apr 26, 2022

zuozp8 commented Apr 26, 2022

devsnek commented Apr 26, 2022

guybedford commented Apr 29, 2022

devsnek commented Apr 29, 2022

mhdawson commented Apr 29, 2022

guybedford commented Apr 29, 2022

bnoordhuis commented May 5, 2022

joyeecheung commented May 5, 2022

joyeecheung commented May 5, 2022 •

edited

Loading

joyeecheung commented Jun 2, 2022

joyeecheung commented Mar 2, 2024 •

edited

Loading

Stalled top-level promise detection for TLA #42868

Stalled top-level promise detection for TLA #42868

Comments

zuozp8 commented Apr 25, 2022 • edited Loading

Version

Subsystem

What steps will reproduce the bug?

What is the expected behavior?

What do you see instead?

RaisinTen commented Apr 25, 2022

zuozp8 commented Apr 25, 2022 • edited Loading

targos commented Apr 25, 2022

RaisinTen commented Apr 26, 2022

JakobJingleheimer commented Apr 26, 2022

RaisinTen commented Apr 26, 2022

zuozp8 commented Apr 26, 2022

devsnek commented Apr 26, 2022

guybedford commented Apr 29, 2022

devsnek commented Apr 29, 2022

mhdawson commented Apr 29, 2022

guybedford commented Apr 29, 2022

bnoordhuis commented May 5, 2022

joyeecheung commented May 5, 2022

joyeecheung commented May 5, 2022 • edited Loading

joyeecheung commented Jun 2, 2022

joyeecheung commented Mar 2, 2024 • edited Loading

zuozp8 commented Apr 25, 2022 •

edited

Loading

zuozp8 commented Apr 25, 2022 •

edited

Loading

joyeecheung commented May 5, 2022 •

edited

Loading

joyeecheung commented Mar 2, 2024 •

edited

Loading