fix: deadlock in ssrLoadModule with circular dependencies (fixes #2491, fixes #2258)

[fairbanksg]

Description

While ssrLoadModule has code to detect circular dependencies, it is possible for some to not be detected.
For example, if two modules directly import each other, and some other module imports both of those, the import path can lead to urlStack not showing the circular dependency. The end result is that the two modules will await each other's pending promises and deadlock.

This change will check that no dependencies of a pending module are in the urlStack, which would lead to a deadlock.

Additionally, the ssrModule is set on the ModuleNode immediately so that the reference is constant, allowing circular dependencies to access exports when they later become available.

Additional context

---

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.
• Read the Pull Request Guidelines and follow the Commit Convention.
• 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).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Shinigami92]

question: Is it possible to provide tests so we prevent regression?

[fairbanksg]

@Shinigami92 I would be happy to add some tests for this, but would appreciate some pointers on the best way to do that for this code.

It seems like I would need to start a ViteDevServer in middleware mode and then call ssrLoadModule with it. I guess I would need to create a temp directory to have some test code in that I could load?

[patak-dev]

@fairbanksg there are currently two ssr playgrounds, ssr-vue and ssr-react. We could create a new playground but I think you could add a test case like the one described in #2491 to one of them. Here is the server calling ssrLoadModule in ssr-vue for example:

vite/packages/playground/ssr-vue/server.js

Line 63 in 06b9935

render = (await vite.ssrLoadModule('/src/entry-server.js')).render

The tests for each playground are in the __tests__ dir: https://github.com/vitejs/vite/blob/main/packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts, in case you need to add a new check. Let me know if you want more info. There is a section about this also here https://github.com/vitejs/vite/blob/main/.github/contributing.md#extending-the-test-suite
If we get a lot of similar cases to test, we may want to split them later into a new playground.

[fairbanksg]

@patak-js I am working on the test for this, but am running into some trouble running the tests.

Even just running them on main with none of my changes, I get failures in both ssr-react and ssr-vue, in the hmr and client navigation tests. Failures attached at the bottom of this comment.

I added some logging, and it seems like the change that is being tested is happening, so I'm not sure why the tests doesn't see it.

These are the steps I took to run the tests.

• yarn install
• yarn build
• yarn test

Summary of all failing tests
 FAIL  packages/playground/ssr-react/__tests__/ssr-react.spec.ts (28.833 s)
  ● hmr

    expect(received).toMatch(expected)

    Expected substring: "changed"
    Received string:    "Home"

      116 |     const actual = (await poll()) || ''
      117 |     if (actual.indexOf(expected) > -1 || tries === maxTries - 1) {
    > 118 |       expect(actual).toMatch(expected)
          |                      ^
      119 |       break
      120 |     } else {
      121 |       await timeout(50)

      at Object.untilUpdated (packages/playground/testUtils.ts:118:22)
      at Object.<anonymous> (packages/playground/ssr-react/__tests__/ssr-react.spec.ts:37:3)

  ● client navigation

    expect(received).toMatch(expected)

    Expected substring: "changed"
    Received string:    "About"

      116 |     const actual = (await poll()) || ''
      117 |     if (actual.indexOf(expected) > -1 || tries === maxTries - 1) {
    > 118 |       expect(actual).toMatch(expected)
          |                      ^
      119 |       break
      120 |     } else {
      121 |       await timeout(50)

      at Object.untilUpdated (packages/playground/testUtils.ts:118:22)
      at Object.<anonymous> (packages/playground/ssr-react/__tests__/ssr-react.spec.ts:47:3)

 FAIL  packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts (29.431 s)
  ● hmr

    expect(received).toMatch(expected)

    Expected substring: "changed"
    Received string:    "Home"

      116 |     const actual = (await poll()) || ''
      117 |     if (actual.indexOf(expected) > -1 || tries === maxTries - 1) {
    > 118 |       expect(actual).toMatch(expected)
          |                      ^
      119 |       break
      120 |     } else {
      121 |       await timeout(50)

      at Object.untilUpdated (packages/playground/testUtils.ts:118:22)
      at Object.<anonymous> (packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts:108:3)

  ● client navigation

    expect(received).toMatch(expected)

    Expected substring: "changed"
    Received string:    "About"

      116 |     const actual = (await poll()) || ''
      117 |     if (actual.indexOf(expected) > -1 || tries === maxTries - 1) {
    > 118 |       expect(actual).toMatch(expected)
          |                      ^
      119 |       break
      120 |     } else {
      121 |       await timeout(50)

      at Object.untilUpdated (packages/playground/testUtils.ts:118:22)
      at Object.<anonymous> (packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts:116:3)

[patak-dev]

There seems to be an issue (probably a race condition) with the SSR tests that makes them fails sometimes. But I don't know if that is what you are hitting here. The suite is running correctly most of the time now.
The steps you took to run the tests are correct. You can also try to only run yarn test-serve or yarn test-build to test only dev or build versions. If you want, you can commit your changes here so we see them in the CI instead of on your local machine.

[fairbanksg]

I added some circular dependencies to the ssr-react playground and verified the tests fail without my changes and pass with them.

[raythurnvoid]

@fairbanksg I've opened a pull request to your branch to improve this fix: fairbanksg#1
I've added a check for a problem i've encountered, i've named it "Forked circular dependency deadlock".

[aleclarson]

Closing in favor of #3950