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

fix(server): should close server after create new server #12379

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

sun0day
Copy link
Member

@sun0day sun0day commented Mar 12, 2023

Description

fix #12375
superseds close #11680

Additional context

After this PR, current server will not be closed if creating new server fails, so we could tell user that server restart failed explicitly.


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 PR Title 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.

@stackblitz
Copy link

stackblitz bot commented Mar 12, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 12, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

The following error happens when saving the config without any changes.

[ENOENT: no such file or directory, stat '/home/vitejs/vite/playground/assets/node_modules/.vite/deps_temp_c5a19aad'] {
  code: 'ENOENT',
  errno: -2,
  path: '/home/vitejs/vite/playground/assets/node_modules/.vite/deps_temp_c5a19aad',
  syscall: 'stat'
}

I guess it's happening because:

  1. the deps_temp directory is created by createServer (but since it already exists the creation is skipped)
  2. the deps_temp directory gets deleted by await server.close()
  3. optimizer fails because deps_temp directory doesn't exist

@bluwy
Copy link
Member

bluwy commented Mar 12, 2023

When starting the new server, with the old server still lingering, wouldn't there be a chance the port would conflict? Maybe we need a callback when the new server's resolveConfig() is finished and then we teardown the old server 🤔

@sun0day
Copy link
Member Author

sun0day commented Mar 12, 2023

When starting the new server, with the old server still lingering, wouldn't there be a chance the port would conflict? Maybe we need a callback when the new server's resolveConfig() is finished and then we teardown the old server 🤔

server.listen is called after server.close, so the port wouldn't conflict. Ideally the restart logic will be that only when a new server instance is created successfully, then the old one should close. Adding resolveConfig() callback can certainly solve #12375, but I think it's better to decouple create and close.

@bluwy
Copy link
Member

bluwy commented Mar 12, 2023

Ah I see, then we could go with this flow then once the issue sapphi brought up is resolved. Thanks for the explanation.

@sun0day
Copy link
Member Author

sun0day commented Mar 12, 2023

I found that deps_temp will be recreated every time when vite creates a new server instance because of loading the new config(even if doesn't change) from vite.config.js. Not sure whether this is a bug or intended. @sapphi-red @bluwy

@@ -1271,8 +1271,11 @@ export async function cleanupDepsCacheStaleDirs(
for (const dirent of dirents) {
if (dirent.isDirectory() && dirent.name.includes('_temp_')) {
const tempDirPath = path.resolve(config.cacheDir, dirent.name)
const { mtime } = await fsp.stat(tempDirPath)
if (Date.now() - mtime.getTime() > MAX_TEMP_DIR_AGE_MS) {
const stats = await fsp.stat(tempDirPath).catch((_) => null)
Copy link
Member Author

@sun0day sun0day Mar 12, 2023

Choose a reason for hiding this comment

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

tempDirPath maybe removed when server.close(), so add a catch to make clean up process work properly, this will solve sapphi-red's issue.

@sun0day
Copy link
Member Author

sun0day commented Mar 12, 2023

https://github.com/vitejs/vite/pull/12379/files#diff-456b3c61f2f19b160b6a5016cb9a180500c4e3b3a9a41a192ca24670637e7706L1282

Error throwed by cleanupDepsCacheStaleDirs will confuse users, do they need to know this error?

If needed, maybe we can add more context info to this error and expose it in debug mode @patak-dev

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

This looks good to me. Let's merge it so it makes it in Vite 4.2, @sun0day already resolved the issue reported by @sapphi-red.

@patak-dev patak-dev merged commit d23605d into vitejs:main Mar 15, 2023
@patak-dev
Copy link
Member

Error thrown by cleanupDepsCacheStaleDirs will confuse users, do they need to know this error?

I don't think users need to be aware of these errors 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite dev server crashes if error is introduced in vite.config.js
4 participants