nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
108.01k stars 29.8k forks source link

Top-level await + dynamic import + cyclic import causes "unsettled TLA" error #55468

Open guyutongxue opened 1 month ago

guyutongxue commented 1 month ago

Version

v22.10.0

Platform

Linux GuyuDebian 6.1.0-25-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.106-3 (2024-08-26) x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Create following files:

Then run node foo.js.

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

Every time.

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

Outputs It works!.

import {} from "./foo.js" doesn't import anything, and foo.js has already been linked, so it should not block the evaluation of bar.js.

What do you see instead?

Warning: Detected unsettled top-level await at file:///home/guyutongxue/Downloads/temp/web/foo.js:1
const bar = await import("./bar.js");

And the process exited with error code 13.

Additional information

Behavior of other runtime:

I'm not sure what is the correct behavior that ECMAScript specification defines.

aduh95 commented 1 month ago

foo.js has already been linked

Sure but linking is only important for static imports, no? Because static imports need to know in advance the exported names of each dependencies.

import {} from "./foo.js" doesn't import anything […], so it should not block the evaluation of bar.js.

The import statement itself is a no-op during evaluation, so it's not blocking anything. However, bar.js source text is never evaluated because it's waiting for its dependencies (i.e. ./foo.js) to be evaluated first, which is blocked on the TLA. My understanding is that the bug in on Bun, and Node.js behavior matches how I interpret the spec.

guyutongxue commented 1 month ago

@aduh95 Thanks for your explanation.

However, bar.js source text is never evaluated because it's waiting for its dependencies to be evaluated first ...

But I'm just a bit confused that, in "static" cyclic dependencies, for example,

// a.js
import { b } from "./b.js";
await new Promise((r) => setTimeout(r, 1000));
console.log("module a loaded");

// b.js
import * as a from "./a.js";
console.log(a);
export const b = 1;

The console.log(a) is executed before console.log("module a loaded"). Seems that b.js does not wait the dependency import ... from "./a.js" to finished. So why things got changed in a dynamic import context?

aduh95 commented 1 month ago

Take the following shell script:

mkdir repro
cd repro
cat -> a.mjs <<'EOF'
import { b } from "./b.mjs";
await new Promise((r) => setTimeout(r, 1000));
console.log("module a loaded");
EOF
cat -> b.mjs <<'EOF'
import * as a from "./a.mjs";
console.log(a);
export const b = 1;
EOF
echo "a.mjs is the entry point:"
node a.mjs
echo
echo "b.mjs is the entry point:"
node b.mjs
cd ..
rm -rf repro

This outputs:

a.mjs is the entry point:
[Module: null prototype] {  }
module a loaded

b.mjs is the entry point:
module a loaded
[Module: null prototype] {  }

As you can see, the order depends on which is the entry point, and the entry point is always executed last.

So why things got changed in a dynamic import context?

Because there's no cyclic module graph when you use dynamic imports. Cyclic module has its own section of the spec, it's not surprising you get different results with static import and dynamic imports.

Jamesernator commented 1 month ago

So why things got changed in a dynamic import context?

This was considered but ultimately rejected as it was favoured that dynamic import should always return a fully evaluated module.

Spec-wise, it wouldn't actually be that complicated to have an option to import(...) to allow partially initialized namespaces (just like static import already allows), but it would need be a TC39 proposal to actually happen.

Technically I think it would actually be conforming for hosts to have this behaviour by an import attribute (e.g. import("./foo.js", { with: { detectCyclicImportAndReturnPartialNamespace: "true" } })).

I don't know if there's really enough demand that Node (let alone other hosts) would add this (though I have seen basically this exact issue be raised a couple times before, so maybe the demand is high enough, but someone would have to implement it).

guyutongxue commented 1 month ago

I've encountered this issue because Vite is building TLA + import() to a cyclic module graph.

Original source:

// main.ts
await import("./module_1");

// module_1.ts
import("./moudle_2");

builds to:

// main.js
export const __vite_preload = /* ... */;
await __vite_preload(() => import("./module_1.js"));

// module_1.js
import { __vite_preload } from "./main.js";
__vite_preload(() => import("./module_2.js"));

The corresponding Vite issue is vitejs/vite#18156 -- this issue does affect a number of user.