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(optimizer): use temp dirs to ensure content of deps cache dir is consistent #12592

Closed
wants to merge 3 commits into from

Conversation

dominikg
Copy link
Contributor

@dominikg dominikg commented Mar 26, 2023

Reasoning:
writing and removing files in the cache dir itself risks leaving a broken state when user aborts the process.

This PR changes it by writing to a temp directory first, and then uses 2 renameSync calls to replace the content.

  1. write new files to /deps_temp_xxxx
  2. move /deps to /deps_remove_yyyy
  3. move /deps_temp_xxx to /deps
  4. delete /deps_remove_yyyy

Description

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 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 26, 2023

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

@dominikg
Copy link
Contributor Author

to avoid buildup of leftover temp directories, vite could delete all deps_remove_* directories on startup, and also all deps_temp_xxxx directories where the xxxx has a timestamp that isn't recent (older than 2h should be sufficient to avoid messing with any process that is still running)

@patak-dev
Copy link
Member

I would like us to try the overwrite approach first and see how it works in the wild. We had a lot of issues with the delete-then-rename scheme in the past year and I think we will end up encountering similar problems with the rename-then-rename-(then-delete-in-the-background).

I will be sending another PR to use the result we have in memory instead of waiting for write-then-read the optimized deps, so we can do the commit in the background. I think the current method will give us fewer issues compared to having many directories.

@dominikg
Copy link
Contributor Author

this is about ensuring content of deps cache on disk being consistent, not really about speed of rereading it. even for async commit you'll want to ensure that deps dir is not left in a half overwritten state if a process is terminated at any given point.

@dominikg
Copy link
Contributor Author

it uses timestamp+random with 10 retries to avoid collisions even with concurrent processes, though these would still end up in a last to finish wins situation, so ultimately we'd need some semaphore or other method to resolve parallel commits by different processes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants