fix(ssr): resolve interlocking circular dependency issues

[shYkiSto]

Description

fixes #11468

Updated logic around managing circular dependencies in ssrModuleLoader (introduced in #3950), which still has been problematic in certain type of scenarios, e.g., when using dynamic imports. This essentially refactors the way Vite keeps track of pending module dependencies using a graph, which helps identify and tolerate circular dependencies more accurately:

• pendingModuleDependencyGraph: Map<string, Set<string>> contains edges, where key is a module url, and the value is a set of URLs of the module's dependencies
• addPendingModuleDependency(originUrl, depUrl) updates the dependency graph, adding an edge representing the interdependency between two modules
• checkModuleDependencyExists(originUrl, targetUrl) checks for the existence of a transitive (or direct) dependency between two modules

All in all this helps detect circular dependencies between modules, allowing modules to be returned partially executed to circumvent initialization deadlocks when two modules are indirectly waiting on one another (aligned w/ the ESM spec). Note that this is only applicable to static imports, as dynamic imports are resolved at runtime and do not contribute to the static dependency graph in the same way.

Added a missing test case capturing a problematic "deadlock" scenario when using dynamic imports, which is currently failing on main: https://github.com/vitejs/vite/actions/runs/7271136773/job/19811284557?pr=15395#step:13:114

Additional context

#11468 became a bespoke umbrella for all sorts of issues when Vite server becomes unresponsive, and while this PR fixes a very particular bug involving circular dependencies, it may not address all of the other issues reported there.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines, especially the Pull Request Guidelines.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[shYkiSto]

Not sure if I should be worried by these two transient test failures, are these known flaky tests perhaps? (they are passing for me locally, and in CI from time to time):

• playground/ssr/__tests__/ssr.spec.ts > html proxy is encoded
  • 🔴 build failed on first run
  • 🟢 build passed on next run
• playground/lib/__tests__/lib.spec.ts [ playground/lib/__tests__/lib.spec.ts ]
  • 🟢 build passed on first run
  • 🔴 build failed on next run

[bluwy]

Seems like generally this changes from a stack to a graph approach for checking circular imports. Can you explain why the stack approach before didn't work before?

I agree that we should probably skip the circular check for dynamic imports. But for static imports the logic seems to be fine before.

[bluwy]

IIUC this code will crawl the dependencies top-down. Would this cause perf issues for many imports?

[shYkiSto]

The traversal should be pretty fast, as the scope is limited to pending modules, e.g., edges are pruned as soon as module finished initializing. And real worst case scenario would be a very dense graph that contains many back-edges, i.e., dependency import in part of the graph that contains cycles. But even in this case the computational overhead is very minimal.

FWIW, I've ran some benchmark on one of the largest projects in our group and cumulative time spent keeping the graph and cycle detection was ~33ms for a module graph w/ ~7.5k nodes and ~36k imports.

[shYkiSto]

Seems like generally this changes from a stack to a graph approach for checking circular imports. Can you explain why the stack approach before didn't work before?

I agree that we should probably skip the circular check for dynamic imports. But for static imports the logic seems to be fine before.

Correct. I believe the original approach using the stack certainly worked just fine for regular (static) imports, where the execution of modules follows sequential order based on current implementation in Vite. But in reality the process may start from multiple distant points in the module graph, e.g., in the dynamic imports case, and it lacks accuracy in this type of scenario so circular references go undetected.

/2c I dunno what future direction is for Vite, but it seems that current approach to handling ES modules in SSR context has diverged from the "native" implementation in both Node.js and browsers (it mimics the CJS more than it does ESM). Which can certainly lead to some inconsistent / unexpected behavior, and may present some challenges in the future. So perhaps it could be one area to focus on in the near-term to ensure the implementations is closely aligned w/ the standard, e.g., maybe it could run natively w/ help of experimental module loaders in Node.js.

[Creskendoll]

I tested this branch with my project that's currently impacted by this bug, and it works like a charm 🎉
This'll save countless projects like mine out there.
Can't thank you enough for your work @shYkiSto

[shYkiSto]

Is there anything I can do to help expedite reviewing of this PR? Would be great to patch this long-standing issue in the next minor 🤞

cc @bluwy @patak-dev

[patrickklaeren]

+1 I believe this is the cause of a hang when we npm run dev.

Disabling SSR for our layout, as suggested in #11468 (comment) works.

[shYkiSto]

Hey @bluwy, @patak-dev,

Any chance this can be prioritized for the upcoming releases? We've been running this revision internally for 3+ months, and there're no reports of circular dependencies causing issues (also, other folks reported that it indeed resolved the issue in their project)

Thanks!

[patak-dev]

Thanks for your patience here @shYkiSto. I added it to the 5.3 milestone so we force us to push us to decide if we'll move forward with this PR.