Skip to content

Lock scratch directory during tool execution #7269

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

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Conversation

neonichu
Copy link
Contributor

Currently, concurrent execution of SwiftPM can easily break the state in .build irreparably. This is especially in conjunction with tools using file watching to trigger package resolution which is something that e.g. the Swift VS Code plugin does.

We can resolve this by locking the .build directory for the duration of execution of any SwiftTool such that any following access will do a blocking wait until unlock. This is using the same infrastructure that we are already using for the shared repository cache. Unlike the shared cache, tool execution can take a long time, so we'll inform the user about the waiting process so that they can decide to cancel instead.

rdar://113964179

Currently, concurrent execution of SwiftPM can easily break the state in .build irreparably. This is especially in conjunction with tools using file watching to trigger package resolution which is something that e.g. the Swift VS Code plugin does.

We can resolve this by locking the .build directory for the duration of execution of any `SwiftTool` such that any following access will do a blocking wait until unlock. This is using the same infrastructure that we are already using for the shared repository cache. Unlike the shared cache, tool execution can take a long time, so we'll inform the user about the waiting process so that they can decide to cancel instead.

rdar://113964179
@neonichu neonichu self-assigned this Jan 18, 2024
@neonichu
Copy link
Contributor Author

I wonder if we need/want to do anything special for non-interactive execution (e.g. always bail if .build is locked) and also potentially a flag like swift build --no-wait-for-scratch-directory-lock.

@neonichu
Copy link
Contributor Author

@neonichu
Copy link
Contributor Author

Not sure I understand the Linux failure:

[980/1149] Compiling Basics AbsolutePath.swift
/home/build-user/swiftpm/Sources/Basics/FileSystem/FileSystem+Extensions.swift:200:36: error: cannot convert value of type 'TSCAbsolutePath' (aka 'AbsolutePath') to expected argument type 'AbsolutePath'
        try self.withLock(on: path.underlying, type: type, blocking: blocking, body)
                                   ^
[981/1149] Compiling Basics FileSystem+Extensions.swift
/home/build-user/swiftpm/Sources/Basics/FileSystem/FileSystem+Extensions.swift:200:36: error: cannot convert value of type 'TSCAbsolutePath' (aka 'AbsolutePath') to expected argument type 'AbsolutePath'
        try self.withLock(on: path.underlying, type: type, blocking: blocking, body)
                                   ^

@neonichu
Copy link
Contributor Author

Not sure I understand the Linux failure

Oh, of course this is because self-hosted jobs don't support cross repo testing.

@neonichu
Copy link
Contributor Author

swiftlang/swift-tools-support-core#458
@swift-ci please test windows

@neonichu
Copy link
Contributor Author

Windows failure looks related:

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-tools-support-core\Sources\TSCBasic\Lock.swift:117:37: error: cannot convert value of type 'Int32' to expected argument type 'Int'
        case .exclusive: dwFlags |= LOCKFILE_EXCLUSIVE_LOCK

@neonichu
Copy link
Contributor Author

swiftlang/swift-tools-support-core#458
@swift-ci please test windows

@shahmishal shahmishal merged commit a8e6870 into main Jan 22, 2024
@shahmishal shahmishal deleted the lock-scratch-directory branch January 22, 2024 17:03
@krzyzanowskim
Copy link
Contributor

I wonder if we need/want to do anything special for non-interactive execution

nice! I proposed to lock a few years ago #2036 (comment), which, unfortunately, was rejected as not needed. I'm happy to see the idea finally made it through.

@neonichu The work is here in case that may be helpful in terms of scope #2037. One thing is that I believe Workspace should do the locking too, like when invoked by the sourcekit-lsp or other such.

neonichu added a commit that referenced this pull request Jan 23, 2024
We're seeing hangs in self-hosted jobs since merging this and I can't
repro locally, so let's try this speculative revert to see whether it
resolves the issues.

Reverts #7269
@neonichu
Copy link
Contributor Author

I wonder if we need/want to do anything special for non-interactive execution (e.g. always bail if .build is locked) and also potentially a flag like swift build --no-wait-for-scratch-directory-lock.

To answer myself here somewhat delayed, it doesn't seem as if we can do anything for non-interactive execution without requiring changes to the VS Code plugin (and potentially many other integrations that are shelling out to SwiftPM). By definition these integrations are non-interactive and if we don't make them wait they would fail in potentially many scenarios that work fine today.

@krzyzanowskim
Copy link
Contributor

it doesn't seem as if we can do anything for non-interactive execution without requiring changes to the VS Code plugin

what exactly? shouldn't sourcekit-lsp at least be aware and avoid command line execution overlap?

@neonichu
Copy link
Contributor Author

The VS Code plugin shells out directly to swift package resolve etc, outside of its use of sourcekit-lsp.

@krzyzanowskim
Copy link
Contributor

that's exactly my point. these two may (and do) overlap without any lock

neonichu added a commit that referenced this pull request Jan 23, 2024
This reverts commit e92df2d and brings back the changes from #7269.
neonichu added a commit that referenced this pull request Jan 29, 2024
This reverts commit e92df2d and brings back the changes from #7269.
neonichu added a commit that referenced this pull request Jan 31, 2024
This reverts commit e92df2d and brings back the changes from #7269.
neonichu added a commit that referenced this pull request Jan 31, 2024
This reverts commit e92df2d and brings back the changes from #7269.

resolves #6643
rdar://113964179
neonichu added a commit that referenced this pull request Jan 31, 2024
This reverts commit e92df2d and brings back the changes from #7269.

resolves #6643
rdar://113964179
neonichu added a commit that referenced this pull request Feb 2, 2024
This reverts commit e92df2d and brings back the changes from #7269.

resolves #6643
rdar://113964179
neonichu added a commit that referenced this pull request Feb 5, 2024
This reverts commit e92df2d and brings back the changes from #7269.

resolves #6643
rdar://113964179
neonichu added a commit that referenced this pull request Feb 5, 2024
This reverts commit e92df2d and brings back the changes from #7269.

resolves #6643
rdar://113964179
neonichu added a commit that referenced this pull request Feb 6, 2024
This reverts commit e92df2d and brings
back the changes from #7269.
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