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.

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

Experimental require of circular ESM graph stack overflow #52864

Closed
laverdet opened this issue May 6, 2024 · 3 comments
Closed

Experimental require of circular ESM graph stack overflow #52864

laverdet opened this issue May 6, 2024 · 3 comments
Labels
loaders Issues and PRs related to ES module loaders

Comments

@laverdet
Copy link
Contributor

laverdet commented May 6, 2024

Version

v22.1.0

Platform

macos

Subsystem

modules

What steps will reproduce the bug?

$ cat left.mjs 
import {} from './right.mjs';

$ cat right.mjs 
import {} from './left.mjs';

$ node left.mjs
# (nothing)

$ cat main.cjs 
require('./left.mjs');

$ node --experimental-require-module main.cjs
node:fs:441
function readFileSync(path, options) {
                     ^

RangeError: Maximum call stack size exceeded
    at readFileSync (node:fs:441:22)
    at getSourceSync (node:internal/modules/esm/load:85:14)
    at defaultLoadSync (node:internal/modules/esm/load:201:32)
    at ModuleLoader.getModuleJobForRequire (node:internal/modules/esm/loader:363:24)
    at new ModuleJobSync (node:internal/modules/esm/module_job:307:32)
    at ModuleLoader.getModuleJobForRequire (node:internal/modules/esm/loader:404:11)
    at new ModuleJobSync (node:internal/modules/esm/module_job:307:32)
    at ModuleLoader.getModuleJobForRequire (node:internal/modules/esm/loader:404:11)
    at new ModuleJobSync (node:internal/modules/esm/module_job:307:32)
    at ModuleLoader.getModuleJobForRequire (node:internal/modules/esm/loader:404:11)

Node.js v22.1.0

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

It reproduces consistently.

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

Circular imports are well-defined in the specification and should work correctly.

What do you see instead?

Stack overflow

Additional information

No response

@laverdet
Copy link
Contributor Author

laverdet commented May 6, 2024

@joyeecheung #51977 thanks!

@GeoffreyBooth
Copy link
Member

@nodejs/loaders

@joyeecheung
Copy link
Member

Thanks for reporting it, fix in #52868

@VoltrexKeyva VoltrexKeyva added the loaders Issues and PRs related to ES module loaders label May 7, 2024
nodejs-github-bot pushed a commit that referenced this issue May 9, 2024
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: #52868
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this issue May 11, 2024
To minimize changes if/when we change the layout of the
result returned by require(esm).

PR-URL: #52868
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this issue May 11, 2024
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: #52868
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
To minimize changes if/when we change the layout of the
result returned by require(esm).

PR-URL: #52868
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
To minimize changes if/when we change the layout of the
result returned by require(esm).

PR-URL: #52868
Fixes: #52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Jun 18, 2024
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sophonieb pushed a commit to sophonieb/node that referenced this issue Jun 20, 2024
To minimize changes if/when we change the layout of the
result returned by require(esm).

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sophonieb pushed a commit to sophonieb/node that referenced this issue Jun 20, 2024
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
To minimize changes if/when we change the layout of the
result returned by require(esm).

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
So that if there are circular dependencies in the synchronous
module graph, they could be resolved using the cached jobs.
In case linking fails and the error gets caught, reset the
cache right after linking. If it succeeds, the caller will
cache it again. Otherwise the error bubbles up to users,
and since we unset the cache for the unlinkable module
the next attempt would still fail.

PR-URL: nodejs#52868
Fixes: nodejs#52864
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants