Skip to content

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

Closed
fairbanksg wants to merge 1 commit intovitejs:mainfrom
fairbanksg:fix-ssr-hang
Closed

fix: deadlock in ssrLoadModule with circular dependencies (fixes #2491, fixes #2258)#3473
fairbanksg wants to merge 1 commit intovitejs:mainfrom
fairbanksg:fix-ssr-hang

Conversation

@fairbanksg
Copy link
Copy Markdown
Contributor

@fairbanksg fairbanksg commented May 19, 2021

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.

@fairbanksg fairbanksg changed the title fix: deadlock in ssrLoadModule with circular dependencies, fix #2491, #2258 fix: deadlock in ssrLoadModule with circular dependencies, fix #2491,#2258 May 19, 2021
@fairbanksg fairbanksg changed the title fix: deadlock in ssrLoadModule with circular dependencies, fix #2491,#2258 fix: deadlock in ssrLoadModule with circular dependencies (fixes #2491, fixes #2258) May 19, 2021
This was linked to issues May 20, 2021
Circular dependencies in SSR not detected and causes indefinite hang #2491
Closed
SSR support circular dependency #2258
Closed
@Shinigami92 Shinigami92 added feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels May 20, 2021
Shinigami92
Shinigami92 previously approved these changes May 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shinigami92 Shinigami92 requested a review from patak-cat May 20, 2021 08:42
@fairbanksg
Copy link
Copy Markdown
Contributor Author

fairbanksg commented May 20, 2021

@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-cat
Copy link
Copy Markdown
Member

patak-cat commented May 20, 2021

@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
Copy link
Copy Markdown
Contributor Author

fairbanksg commented Jun 4, 2021

@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-cat
Copy link
Copy Markdown
Member

patak-cat commented Jun 5, 2021

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
Copy link
Copy Markdown
Contributor Author

fairbanksg commented Jun 7, 2021

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

@raythurnvoid
Copy link
Copy Markdown
Contributor

raythurnvoid commented Jun 22, 2021 •
edited
Loading

@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 added a commit to aleclarson/vite that referenced this pull request Jun 24, 2021
@aleclarson
chore: revert ssrModuleLoader changes from vitejs#3473
88d44c0
@aleclarson
Copy link
Copy Markdown
Contributor

aleclarson commented Jun 24, 2021

Closing in favor of #3950

@aleclarson aleclarson closed this Jun 24, 2021
aleclarson added a commit to aleclarson/vite that referenced this pull request Jul 16, 2021
@aleclarson
chore: revert ssrModuleLoader changes from vitejs#3473
96bfd46
aleclarson added a commit to aleclarson/vite that referenced this pull request Jul 29, 2021
@aleclarson
chore: revert ssrModuleLoader changes from vitejs#3473
25eb604
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat Awaiting requested review from patak-cat

1 more reviewer

@Shinigami92 Shinigami92 Shinigami92 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feat: ssr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Circular dependencies in SSR not detected and causes indefinite hang SSR support circular dependency

5 participants

@fairbanksg @patak-cat @raythurnvoid @aleclarson @Shinigami92