Repository navigation
[Tool] Fix SwiftPM race condition during parallel Xcode builds - #188451
Conversation
Xcode builds multi-target applications in parallel, invoking the Flutter build pipeline concurrently. This leads to a destructive race condition in `generatePluginsSwiftPackage` where one process deletes the ephemeral packages directory while another is writing to it, causing `FileSystemException` or `PathExistsException`. This change implements: 1. Process-safe file locking (`.swift_pm.lock`) with retry loops to serialize directory preparation across parallel builds. 2. Non-destructive, incremental cleanup of obsolete symlinks rather than deleting the entire directory. 3. Content-aware write skipping for `Package.swift` and placeholder source files to avoid redundant writes and prevent unnecessary Xcode project re-indexing. Fixes flutter#188446
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
Implement `ephemeralDirectory` on `FakeMacOSProject` and `FakeIosProject` inside `cocoapod_utils_test.dart` to fix test failures introduced by the SwiftPM locking mechanism.
…ter into fix-swiftpm-concurrency-race
Restore the check to skip writing the placeholder `TargetName.swift` if the target directory already contains other source files. Optimized to only perform `listSync()` when the placeholder file does not exist, avoiding file system overhead during incremental builds where it is already present.
Always skip writing the dummy empty source template if the required Swift file already exists in `createSwiftPackage`. Previously, the tool would overwrite any existing source file that had the name `<target>.swift` (such as `FlutterToolHelper.swift`) if its content didn't match the dummy empty template. This caused template-generated source files in the Swift package to be destroyed and replaced with the empty placeholder, breaking compilation in macOS integration tests.
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency control and optimization to the Swift Package Manager integration in Flutter. It implements a file-locking mechanism (.swift_pm.lock) to prevent concurrent processes from corrupting the packages directory, cleans up stale symlinks non-destructively, and optimizes file writes by skipping generation when Package.swift or placeholder source files are unchanged. The reviewer suggests adding a timeout to the lock acquisition loop to prevent potential infinite hangs during build execution.
Extract shared locking logic into a unified `FileSystem.runLocked` extension method in packages/flutter_tools. Fixes two critical bugs in `runLocked`: - Moves the execution of `scope()` outside of the lock acquisition retry loop. This prevents an infinite retry loop if the callback throws a `FileSystemException`. - Saves the file open state in a local variable before closing and nullifying it, ensuring the trace details log message correctly appends the exception details only on open failures. Simplifies `SwiftPackageManager` using the new `runLocked` API.
Add a doc comment explaining that `_cleanStaleSymlinks` is responsible for removing stale or unreferenced plugin symlinks from the Xcode runner's symlinks directory when project dependencies change. This clarifies why the cleanup is necessary to prevent broken link errors during build.
There was a problem hiding this comment.
Code Review
This pull request introduces a file locking mechanism (runLocked) on FileSystem to synchronize Swift Package Manager operations and prevent concurrent modification issues. It updates SwiftPackageManager to run package generation within this lock, cleans up stale symlinks, and optimizes Package.swift generation by avoiding rewrites when the content is identical. Feedback suggests adding a timeout or retry limit to the lock acquisition loop in runLocked to prevent potential infinite loops or build hangs in environments with persistent file system issues.
|
FYI @vashworth, this is ready for review. |
| if (!hasSources) { | ||
| requiredSwiftFile.createSync(recursive: true); | ||
| requiredSwiftFile.writeAsStringSync(_swiftPackageSourceTemplate); | ||
| } |
There was a problem hiding this comment.
why was this refactor required?
vashworth
left a comment
There was a problem hiding this comment.
Overall, LGTM, but we may want to have a discussion on why this is happening and how to fix it in a broader sense. Doing all these workarounds specific to certain parts of code seems fragile.
…dart Add inline comments in explaining why and placeholder file creation is skipped when contents are unchanged. This preserves file modification timestamps () and prevents Xcode and Swift Package Manager from invalidating caches during parallel builds.
Xcode builds multi-target applications in parallel, invoking the Flutter build pipeline concurrently. This leads to a destructive race condition in
generatePluginsSwiftPackagewhere one process deletes the ephemeral packages directory while another is writing to it, causingFileSystemExceptionorPathExistsException.This change implements:
Process-safe file locking (
.swift_pm.lock) with retry loops to serialize directory preparation across parallel builds.Non-destructive, incremental cleanup of obsolete symlinks rather than deleting the entire directory.
Content-aware write skipping for
Package.swiftand placeholder source files to avoid redundant writes and prevent unnecessary Xcode project re-indexing.Fixes #188446