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): check .vite/deps directory existence before removing #11499

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

jeromehan
Copy link
Contributor

@jeromehan jeromehan commented Dec 27, 2022

Potential fix for #9986

@jeromehan jeromehan changed the title fix: fixed ENOENT: no such file or directory #9986 fix: fixed ENOENT: no such file or directory (#9986) Dec 28, 2022
@bluwy bluwy changed the title fix: fixed ENOENT: no such file or directory (#9986) fix(optimizer): check .vite/deps directory existence before removing Dec 28, 2022
@benmccann
Copy link
Collaborator

This workaround still seems susceptible to race conditions even if it causes them to occur less frequently

@bluwy
Copy link
Member

bluwy commented Dec 28, 2022

This workaround still seems susceptible to race conditions even if it causes them to occur less frequently

Are you referring to the race condition within the optimizer in general? I think @patak-dev solved that one before. This fix seems like the likely reason why it still happens even with Patak's fix though.

@benmccann
Copy link
Collaborator

I believe the root cause of #9986 is that the optimizer can run twice in parallel and so trying to do the rename twice fails (#9986 (comment)). I think there'd still be some possibility for that to occur even after this PR. It seems like this code can't be written in a way that's fully blocking because gracefulRename calls setTimeout which will invoke the event loop. Because processing is handed off to the event loop, the other optimizer job can attempt the rename at the same time causing a failure.

I think the real fix would be to prevent the optimizer from running twice in parallel. That seems like it would also be better for performance as well if we can cancel the initial job (evanw/esbuild#2725). It's also a more involved fix though

I'm not sure what fix of @patak-dev's you're referring to. As far as I know, nothing has been fixed, but I don't follow development nearly as closely as you do, so very well may have missed it.

@patak-dev
Copy link
Member

This is the PR that @bluwy is referencing:

I think we can still merge this PR, but avoid closing the linked issue. Let's first review avoiding running the optimizer in parallel.

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.

4 participants