Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrong ERR_REQUIRE_CYCLE_MODULE for require(esm) #55516

Closed
nicolo-ribaudo opened this issue Oct 24, 2024 · 1 comment · Fixed by #55520
Closed

Wrong ERR_REQUIRE_CYCLE_MODULE for require(esm) #55516

nicolo-ribaudo opened this issue Oct 24, 2024 · 1 comment · Fixed by #55520
Labels
loaders Issues and PRs related to ES module loaders

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Oct 24, 2024 •
edited
Loading

Version

23.0.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

// main.cjs

try {
  require("./dep.mjs");
} catch {
  require("./dep.mjs");
}
// dep.mjs
await 0;

How often does it reproduce? Is there a required condition?

/

What is the expected behavior? Why is that the expected behavior?

It should throw ERR_REQUIRE_ASYNC_MODULE

What do you see instead?

It throws ERR_REQUIRE_CYCLE_MODULE

Additional information

Probably all the places that currently assert that a module is either not loaded or already evaluated should be changed to check that, the module is either not loaded, evaluated, or instantaited.

cc @joyeecheung
@RedYetiDev RedYetiDev added the loaders Issues and PRs related to ES module loaders label Oct 24, 2024
@nicolo-ribaudo nicolo-ribaudo mentioned this issue Oct 24, 2024
Merged
@joyeecheung joyeecheung mentioned this issue Oct 24, 2024
Merged
@joyeecheung
Copy link
Member

joyeecheung commented Oct 24, 2024 •
edited
Loading

Thanks for reporting it, fix in #55520

nodejs-github-bot pushed a commit that referenced this issue Oct 29, 2024
@joyeecheung
module: fix error thrown from require(esm) hitting TLA repeatedly
8aac7da 
This tracks the asynchronicity in the ModuleWraps when they turn out to
contain TLA after instantiation, and throw the right error
(ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes
the freezing of ModuleWraps since it's not meaningful to freeze
this when the rest of the module loader is mutable, and we
can record the asynchronicity in the ModuleWrap right after
compilation after we get a V8 upgrade that contains
v8::Module::HasTopLevelAwait() instead of searching through
the module graph repeatedly which can be slow.

PR-URL: #55520
Fixes: #55516
Refs: #52697
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Oct 30, 2024
@joyeecheung @Ceres6
module: fix error thrown from require(esm) hitting TLA repeatedly
8f6778c 
This tracks the asynchronicity in the ModuleWraps when they turn out to
contain TLA after instantiation, and throw the right error
(ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes
the freezing of ModuleWraps since it's not meaningful to freeze
this when the rest of the module loader is mutable, and we
can record the asynchronicity in the ModuleWrap right after
compilation after we get a V8 upgrade that contains
v8::Module::HasTopLevelAwait() instead of searching through
the module graph repeatedly which can be slow.

PR-URL: nodejs#55520
Fixes: nodejs#55516
Refs: nodejs#52697
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this issue Nov 1, 2024
@joyeecheung @RafaelGSS
module: fix error thrown from require(esm) hitting TLA repeatedly
d3be3da 
This tracks the asynchronicity in the ModuleWraps when they turn out to
contain TLA after instantiation, and throw the right error
(ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes
the freezing of ModuleWraps since it's not meaningful to freeze
this when the rest of the module loader is mutable, and we
can record the asynchronicity in the ModuleWrap right after
compilation after we get a V8 upgrade that contains
v8::Module::HasTopLevelAwait() instead of searching through
the module graph repeatedly which can be slow.

PR-URL: #55520
Fixes: #55516
Refs: #52697
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@joyeecheung @louwers
module: fix error thrown from require(esm) hitting TLA repeatedly
07e1ce7 
This tracks the asynchronicity in the ModuleWraps when they turn out to
contain TLA after instantiation, and throw the right error
(ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes
the freezing of ModuleWraps since it's not meaningful to freeze
this when the rest of the module loader is mutable, and we
can record the asynchronicity in the ModuleWrap right after
compilation after we get a V8 upgrade that contains
v8::Module::HasTopLevelAwait() instead of searching through
the module graph repeatedly which can be slow.

PR-URL: nodejs#55520
Fixes: nodejs#55516
Refs: nodejs#52697
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
loaders Issues and PRs related to ES module loaders
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

module: fix error thrown from require(esm) hitting TLA repeatedly joyeecheung/node
3 participants
@joyeecheung @nicolo-ribaudo @RedYetiDev