-
Notifications
You must be signed in to change notification settings - Fork 30.3k
Turbopack: Tweak retry loop for link creation to try to fix os error 80 on Windows #88669
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
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
b8221f9 to
5e9918c
Compare
823a5c3 to
8964035
Compare
Stats from current PR✅ No significant changes detected📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles: **432 kB** → **432 kB** ✅ -49 B82 files with content-based hashes (individual files not comparable between builds) Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
|
8964035 to
17902e9
Compare
5e9918c to
0c72ce0
Compare
17902e9 to
40bb0f1
Compare
0c72ce0 to
861a68b
Compare
Tests Passed |
b302330 to
8e9c123
Compare
6e3f09c to
671c1df
Compare
| } else { | ||
| std::os::windows::fs::symlink_file(&target, &full_path) | ||
| }; | ||
| io_result.map_err(|err| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… string formatting (#88668) - #87661 removed the `tokio::task::spawn_blocking` call from `retry_blocking`, so we can simplify things a lot by removing the `Send + 'static` bounds, and eliminate a bunch of cloning `Path`s into `PathBuf`s. - `retry_blocking` no longer takes a path argument. After removing the path cloning requirements, it's now easier for callers to capture paths via a closure, and it's more flexible for IO operations that need to pass in more than a single path. - Adds `retry_blocking_custom`, which I use in #88669 that allows customizing what error kinds we retry on. - Adds documentation to `retry_blocking` explaining why we need this. It's often unclear to new people on the team why we have to retry filesystem operations (Windows, basically). This was unclear to me when I was new to this code, and other people have asked similar questions. - We shouldn't use `Path::display` for error messages or tracing spans. It's easier to use `Debug`, and `Debug` ensures that if the error is related to unicode, that information isn't lost in the error message. - More consistently apply tracing spans using `.instrument`. - Use the modern inline syntax for some format strings. - `let foo = RcStr::from(bar)` is preferred over `let foo: RcStr = bar.into()`
2554f63 to
f263c56
Compare
671c1df to
771b8da
Compare

Though I've been unable to reproduce the issue (prior PRs in this stack are my attempts at that), I believe this should fix the OS Error 80 (
ERROR_FILE_EXISTS) that we've seen users report on Windows: #88382Basically instead of deleting the existing link in a retry loop, and then trying to create new new link using a retry loop, we should instead:
ErrorKind::AlreadyExists. This might be possible if the filesystem is eventually consistent (which can happen on Windows), but I wasn't able to reproduce this.There's a similar -- but different -- issue that users have been reporting where they get OS Error 1 (
ERROR_INVALID_FUNCTION) on Windows. I believe this happens if we try to create a junction point on a non-NTFS filesystem. In that case, we need to report a clearer issue to the user instead of bubbling this up as an internal Turbopack error.I tested this on Windows with the symlink fuzzer from #88667, as well as
cargo test -p turbo-tasks-fs: