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

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/

lib/internal/modules/esm/loader.js

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

lib/internal/modules/esm/module_job.js

lib/internal/modules/run_main.js

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

src/env.cc

+  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

lib/internal/modules/esm/loader.js

-    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

JakobJingleheimer

Huzzah! Thanks for this @joyeecheung

JakobJingleheimer · 2024-03-10T09:35:40Z

lib/internal/modules/esm/loader.js

+
+/**
+ * This is a singleton ESM loader that integrates the loader hooks, if any.
+ * It it used by other internal built-ins when they need to load ESM code


Suggested change

* It it used by other internal built-ins when they need to load ESM code

* It is used by other internal built-ins when they need to load ESM code

JakobJingleheimer · 2024-03-10T09:40:26Z

lib/internal/process/execution.js

+ * it finishes evaluation.
+ * @param {string} source Source code the ESM
+ * @param {boolean} print Whether the result should be printed.
+ * @returns {Promise}


nit: what does the promise resolve to?

JakobJingleheimer · 2024-03-10T09:44:35Z

lib/internal/modules/esm/loader.js

@@ -568,6 +561,23 @@ function getHooksProxy() {
  return hooksProxy;
 }

+let cascadedLoader;


Out of curiosity: why call it "cascaded" loader?

JakobJingleheimer · 2024-03-10T09:46:37Z

lib/internal/modules/esm/loader.js

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


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

aduh95 · 2024-03-10T11:10:01Z

Marking it as baking-for-lts PRs that need to wait before landing in a LTS release. in case the added output breaks someone.

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>

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

lib/internal/modules/esm/loader.js Show resolved Hide resolved

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

joyeecheung closed this Mar 10, 2024

JakobJingleheimer reviewed Mar 10, 2024

View reviewed changes

aduh95 added the baking-for-lts PRs that need to wait before landing in a LTS release. label Mar 10, 2024

joyeecheung mentioned this pull request Mar 19, 2024

Disallow ESM-CJS-ESM-ESM cycles when using require(esm) #52145

Closed

RafaelGSS mentioned this pull request Apr 15, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joyeecheung commented Mar 7, 2024 •

edited

nodejs-github-bot commented Mar 7, 2024

nodejs-github-bot commented Mar 7, 2024

MoLow left a comment

targos commented Mar 7, 2024

aduh95 Mar 7, 2024

joyeecheung Mar 7, 2024 •

edited

aduh95 Mar 7, 2024 •

edited

nodejs-github-bot commented Mar 7, 2024

nodejs-github-bot commented Mar 7, 2024

GeoffreyBooth Mar 9, 2024

joyeecheung Mar 10, 2024

JakobJingleheimer Mar 10, 2024

joyeecheung commented Mar 10, 2024

JakobJingleheimer left a comment

JakobJingleheimer Mar 10, 2024

JakobJingleheimer Mar 10, 2024

JakobJingleheimer Mar 10, 2024

JakobJingleheimer Mar 10, 2024

aduh95 commented Mar 10, 2024

	* It it used by other internal built-ins when they need to load ESM code
	* It is used by other internal built-ins when they need to load ESM code

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

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

Conversation

joyeecheung commented Mar 7, 2024 • edited

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

nodejs-github-bot commented Mar 7, 2024

nodejs-github-bot commented Mar 7, 2024

MoLow left a comment

Choose a reason for hiding this comment

targos commented Mar 7, 2024

Choose a reason for hiding this comment

joyeecheung Mar 7, 2024 • edited

Choose a reason for hiding this comment

aduh95 Mar 7, 2024 • edited

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 7, 2024

nodejs-github-bot commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Mar 10, 2024

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Mar 10, 2024

joyeecheung commented Mar 7, 2024 •

edited

joyeecheung Mar 7, 2024 •

edited

aduh95 Mar 7, 2024 •

edited