module: print location of unsettled top-level await in entry points by joyeecheung · Pull Request #51999 · nodejs/node

joyeecheung · 2024-03-07T05:11:27Z

module: refactor ESM loader initialization and entry point handling

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.

src: refactor out FormatErrorMessage for error formatting

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

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 when the root module of the graph is
also the entry point of the Node.js instance. Other kinds of root
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.

Fixes: #42868

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.

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.

nodejs-github-bot · 2024-03-07T05:11:32Z

Review requested:

@nodejs/loaders
@nodejs/test_runner
@nodejs/vm

nodejs-github-bot · 2024-03-07T05:17:37Z

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

MoLow

JS changes LGTM

targos · 2024-03-07T07:39:50Z

Codeowners lint:

==> Executing File Exist Checker (213.485787ms)
[err] line 99: "/lib/internal/process/esm_loader.js" does not match any files in repository

aduh95 · 2024-03-07T08:19:11Z

+  CHECK_EQ(object->InternalFieldCount(),
+           loader::ModuleWrap::kInternalFieldCount);
+  auto* wrap = BaseObject::FromJSObject<loader::ModuleWrap>(object);
+  return wrap->CheckUnsettledTopLevelAwait();


Should we add an assert here that we expect wrap->CheckUnsettledTopLevelAwait() to not return v8::Just(true)?

It can return true, for example if it's not a source text module (and therefore it's not caused by TLA per-se), or any other situations where V8 is not able to find it. This is only to help debugging anyway, crashing here is not going to help anyone.

I thought would could get into this branch only if there was some unsettled TLA (because of the various if above), so CheckUnsettledTopLevelAwait would return false only if there was a bug somewhere

This is only to help debugging anyway, crashing here is not going to help anyone.

I guess it depends if getting report would help us improve the detection or not. If you think it would not, I agree we don't need the assert

nodejs-github-bot · 2024-03-07T15:18:18Z

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

…oints

nodejs-github-bot · 2024-03-07T16:54:53Z

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

…dling

GeoffreyBooth · 2024-03-09T06:45:46Z

-    source,
-    url = pathToFileURL(`${process.cwd()}/[eval${++this.evalIndex}]`).href,
-  ) {
+  async eval(source, url, isEntryPoint = false) {


Not sure why this method lacks a JSDoc, do you mind adding one?

Hmm, didn't see this, feel free to follow up and add a comment if you like.

Weird! I just looked through the history, and it seems this method never got a jsdoc when pretty much everything else did.

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>

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>

joyeecheung · 2024-03-10T00:24:00Z

Landed in 257f322...575ced8

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>

targos · 2024-05-08T14:42:42Z

@aduh95 this landed on v20.13.0 by mistake. Should we revert?

marco-ippolito · 2024-05-08T14:44:51Z

only a61e550 and 74221f8 landed on v20.13.0

- nodejs/node#51999 - nodejs/node#51927

nodejs/node#51999

* chore: bump node in DEPS to v20.13.0 * crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL nodejs/node#52217 * test: skip test for dynamically linked OpenSSL nodejs/node#52542 * lib, url: add a `windows` option to path parsing nodejs/node#52509 * src: use dedicated routine to compile function for builtin CJS loader nodejs/node#52016 * test: mark test as flaky nodejs/node#52671 * build,tools: add test-ubsan ci nodejs/node#46297 * src: preload function for Environment nodejs/node#51539 * chore: fixup patch indices * deps: update c-ares to 1.28.1 nodejs/node#52285 * chore: handle updated filenames - nodejs/node#51999 - nodejs/node#51927 * chore: bump node in DEPS to v20.13.1 * events: extract addAbortListener for safe internal use nodejs/node#52081 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * fs: add stacktrace to fs/promises nodejs/node#49849 * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.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>

nodejs/node#51999

* chore: bump node in DEPS to v20.13.1 * chore: bump node in DEPS to v20.14.0 * crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL nodejs/node#52217 * test: skip test for dynamically linked OpenSSL nodejs/node#52542 * lib, url: add a `windows` option to path parsing nodejs/node#52509 * src: use dedicated routine to compile function for builtin CJS loader nodejs/node#52016 * test: mark test as flaky nodejs/node#52671 * build,tools: add test-ubsan ci nodejs/node#46297 * src: preload function for Environment nodejs/node#51539 * chore: fixup patch indices * deps: update c-ares to 1.28.1 nodejs/node#52285 * chore: handle updated filenames * events: extract addAbortListener for safe internal use nodejs/node#52081 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * fs: add stacktrace to fs/promises nodejs/node#49849 * chore: fixup patch indices --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* chore: bump node in DEPS to v20.13.1 * chore: bump node in DEPS to v20.14.0 * chore: update build_add_gn_build_files.patch * chore: update patches * chore: update patches * build: encode non-ASCII Latin1 characters as one byte in JS2C nodejs/node#51605 * crypto: use EVP_MD_fetch and cache EVP_MD for hashes nodejs/node#51034 * chore: update filenames.json * chore: update patches * src: support configurable snapshot nodejs/node#50453 * test: remove test-domain-error-types flaky designation nodejs/node#51717 * src: avoid draining platform tasks at FreeEnvironment nodejs/node#51290 * chore: fix accidentally deleted v8 dep * lib: define FormData and fetch etc. in the built-in snapshot nodejs/node#51598 * chore: remove stray log * crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL nodejs/node#52217 * test: skip test for dynamically linked OpenSSL nodejs/node#52542 * lib, url: add a `windows` option to path parsing nodejs/node#52509 * src: use dedicated routine to compile function for builtin CJS loader nodejs/node#52016 * test: mark test as flaky nodejs/node#52671 * build,tools: add test-ubsan ci nodejs/node#46297 * src: preload function for Environment nodejs/node#51539 * deps: update c-ares to 1.28.1 nodejs/node#52285 * chore: fixup * events: extract addAbortListener for safe internal use nodejs/node#52081 * module: print location of unsettled top-level await in entry points nodejs/node#51999 * fs: add stacktrace to fs/promises nodejs/node#49849 * chore: fixup indices --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Cheng <zcbenz@gmail.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

nodejs/node#51999

joyeecheung added 3 commits March 7, 2024 06:07

src: refactor out FormatErrorMessage for error formatting

74221f8

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 7, 2024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

MoLow reviewed Mar 7, 2024

View reviewed changes

Comment thread lib/internal/modules/esm/loader.js

MoLow approved these changes Mar 7, 2024

View reviewed changes

aduh95 reviewed Mar 7, 2024

View reviewed changes

benjamingr approved these changes Mar 7, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

fixup! module: print location of unsettled top-level await in entry p…

a3b96bb

…oints

joyeecheung force-pushed the tla-message branch from c21c705 to a3b96bb Compare March 7, 2024 16:46

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024

fixup! module: refactor ESM loader initialization and entry point han…

dc1e09d

…dling

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 8, 2024

GeoffreyBooth approved these changes Mar 9, 2024

View reviewed changes

RafaelGSS mentioned this pull request Apr 15, 2024

2024-04-24, Version 22.0.0 (Current) #52505

Merged

marco-ippolito mentioned this pull request May 2, 2024

v20.13.0 proposal #52793

Merged

marco-ippolito mentioned this pull request May 8, 2024

[v20.x] Revert "module: print location of unsettled top-level await in entry points" #52898

Closed

codebytere added a commit to electron/electron that referenced this pull request May 8, 2024

chore: handle updated filenames

f3b22bf

- nodejs/node#51999 - nodejs/node#51927

codebytere added a commit to electron/electron that referenced this pull request May 10, 2024

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

0059ab0

nodejs/node#51999

codebytere added a commit to electron/electron that referenced this pull request May 31, 2024

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

59cc251

nodejs/node#51999

codebytere added a commit to electron/electron that referenced this pull request May 31, 2024

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

40e214f

nodejs/node#51999

joyeecheung mentioned this pull request Jun 17, 2024

module: support require()ing synchronous ESM graphs #51977

Closed

codebytere added a commit to electron/electron that referenced this pull request Oct 16, 2024

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

8ce8b91

nodejs/node#51999

codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024

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

116e3c1

nodejs/node#51999

codebytere added a commit to electron/electron that referenced this pull request Oct 21, 2024

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

bb61f87

nodejs/node#51999

codebytere added a commit to electron/electron that referenced this pull request Oct 22, 2024

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

Uh oh!

Conversation

joyeecheung commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

module: refactor ESM loader initialization and entry point handling

src: refactor out FormatErrorMessage for error formatting

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

Uh oh!

nodejs-github-bot commented Mar 7, 2024

Uh oh!

nodejs-github-bot commented Mar 7, 2024

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Mar 7, 2024

Uh oh!

Uh oh!

Uh oh!

aduh95 Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 7, 2024

Uh oh!

nodejs-github-bot commented Mar 7, 2024

Uh oh!

GeoffreyBooth Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 10, 2024

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Mar 10, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 10, 2024

Uh oh!

targos commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marco-ippolito commented May 8, 2024

Uh oh!

joyeecheung commented Mar 7, 2024 •

edited

Loading

joyeecheung Mar 7, 2024 •

edited

Loading

aduh95 Mar 7, 2024 •

edited

Loading

targos commented May 8, 2024 •

edited

Loading