Avoid using `temp_await` on `loadRootPackage` #7009

MaxDesiatov · 2023-10-16T14:33:29Z

Motivation:

Using locks to convert asynchronous functions to synchronous is dangerous, since this can deadlock the Swift Concurrency thread pool. Unfortunately, temp_await (AKA tsc_await) function utilizes exactly this approach.

As SourceKit-LSP starts adopting Swift Concurrency (swiftlang/sourcekit-lsp#850), we should avoid using temp_await in our codebase and make the callers of corresponding async or callback-taking functions themselves async. Otherwise there's a risk of deadlocks in SourceKit-LSP itself, since their Swift Concurrency tasks may be blocked by our uses of temp_await under the hood. We have ~60 uses of temp_await in our codebase, carefully removing them in small portions will reduce the potential disruption.

Modifications:

loadRootPackage already has an async overload in Workspace. Converted callers in SwiftPM commands to use AsyncSwiftCommand protocol instead of SwiftCommand. Also, test cases were made async too.

Result:

Reduced the number of temp_await uses in Source.

MaxDesiatov · 2023-10-16T14:33:57Z

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -8803,13 +8800,15 @@ final class WorkspaceTests: XCTestCase {

        let observability = ObservabilitySystem.makeForTesting()
        let wks = try workspace.getOrCreateWorkspace()
-        XCTAssertNoThrow(try temp_await {


XCTAssertNoThrow doesn't support async 🥲

MaxDesiatov · 2023-10-16T14:34:06Z

@swift-ci test

MaxDesiatov · 2023-10-16T14:57:32Z

@swift-ci test

MaxDesiatov · 2023-10-16T15:23:04Z

@swift-ci test

MaxDesiatov · 2023-10-16T15:27:23Z

@swift-ci test

MaxDesiatov · 2023-10-16T15:32:49Z

@swift-ci test

MaxDesiatov · 2023-10-16T15:42:34Z

@swift-ci test windows

Follow up to #7009, which provides the detailed motivation: we should avoid `temp_await` as it can lead to deadlocks when combined with Swift Concurrency.

MaxDesiatov · 2023-10-17T18:05:41Z

@compnerd would you be able to confirm that this change is fine from the perspective of Swift Concurrency support on Windows?

compnerd · 2023-10-17T18:13:50Z

@compnerd would you be able to confirm that this change is fine from the perspective of Swift Concurrency support on Windows?

I think that as long as we restrict this to 5.10/5.11 it should be okay. Of course we should run through the test suite as well.

MaxDesiatov · 2023-10-17T19:29:43Z

There are no plans to backport it to 5.9. How far are we from enabling the test suite on Windows CI? Until then is the only way to test it on Windows to build latest 5.10 locally and to run swift test locally too?

MaxDesiatov · 2023-12-07T21:33:07Z

@swift-ci test

MaxDesiatov · 2023-12-07T21:38:44Z

@swift-ci test

Follow up to #7009, which provides the detailed motivation: we should avoid `temp_await` as it can lead to deadlocks when combined with Swift Concurrency. --------- Co-authored-by: Yim Lee <yim_lee@apple.com>

MaxDesiatov · 2023-12-11T21:01:00Z

@swift-ci test

MaxDesiatov · 2023-12-12T14:47:14Z

@swift-ci test windows

AndrewHoos

It all looks great except for checkPrecomputeResolution. I don't see why the checkPrecomputeResolution method needs to have a closure. Closures normally are for async callbacks, context managers, or optional extra validation (e.g. assert throws). It seems like that API should get reworked to by natively async

Sources/SPMTestSupport/MockWorkspace.swift

MaxDesiatov · 2023-12-12T15:50:26Z

@swift-ci test

MaxDesiatov · 2023-12-12T15:50:41Z

@swift-ci test windows

AndrewHoos · 2023-12-12T15:56:28Z

Sources/Commands/PackageTools/Format.swift

@@ -71,7 +69,7 @@ extension SwiftPackageTool {
            let args = [swiftFormat.pathString] + formatOptions + [packagePath.pathString] + paths
            print("Running:", args.map{ $0.spm_shellEscaped() }.joined(separator: " "))

-            let result = try TSCBasic.Process.popen(arguments: args)
+            let result = try await TSCBasic.Process.popen(arguments: args)


I haven't looked, but I wonder if this is actually non-blocking

Good catch, I'm cleaning that up on TSC side now.

Wrapped that with a concurrent queue in swiftlang/swift-tools-support-core#450

🤣 ask me if I have deadlocked the swift concurrency pool before...

Do you want to use the safe_async wrapper until that lands?

I've enabled auto-merge on that one, so I don't think there will be a significant amount of time when this PR is merged, but one in TSC isn't.

MaxDesiatov · 2023-12-12T16:05:49Z

@swift-ci test windows

MaxDesiatov · 2023-12-12T16:32:12Z

@swift-ci test windows

Follow up to #7009, which provides the detailed motivation: we should avoid `temp_await` as it can lead to deadlocks when combined with Swift Concurrency.

This reverts commit 6bed023.

MaxDesiatov added the concurrency label Oct 16, 2023

MaxDesiatov requested review from neonichu and tomerd as code owners October 16, 2023 14:33

MaxDesiatov commented Oct 16, 2023

View reviewed changes

MaxDesiatov self-assigned this Oct 16, 2023

MaxDesiatov mentioned this pull request Oct 17, 2023

Avoid use of temp_await in PackageRegistryToolTests.swift #7015

Merged

MaxDesiatov added a commit that referenced this pull request Oct 17, 2023

Avoid using temp_await in SwiftTestTool.swift

4a75bf0

Follow up to #7009, which provides the detailed motivation: we should avoid `temp_await` as it can lead to deadlocks when combined with Swift Concurrency.

MaxDesiatov mentioned this pull request Oct 17, 2023

Avoid using temp_await in SwiftTestTool.swift #7016

Merged

MaxDesiatov requested a review from compnerd October 17, 2023 18:05

MaxDesiatov added the next waiting for next merge window label Oct 19, 2023

MaxDesiatov removed the next waiting for next merge window label Nov 28, 2023

MaxDesiatov added 6 commits December 7, 2023 21:37

Avoid using temp_await on loadRootPackage

7c8b716

Fix missing AsyncParsableCommand conformance

041488d

Fix missing await in SwiftPM.swift

cd251a1

Fix missing @main entrypoint in swift-package executable

417a974

Make DumpPackage conform to AsyncSwiftCommand

82070c5

Remove uses of temp_await in MockWorkspace

1d5922d

MaxDesiatov force-pushed the maxd/async-load-package branch from 7b28181 to 1d5922d Compare December 7, 2023 21:37

Merge branch 'main' into maxd/async-load-package

5449056

MaxDesiatov requested a review from AndrewHoos December 12, 2023 14:47

MaxDesiatov enabled auto-merge (squash) December 12, 2023 14:48

AndrewHoos requested changes Dec 12, 2023

View reviewed changes

Sources/SPMTestSupport/MockWorkspace.swift Show resolved Hide resolved

Address PR feedback

afb6316

MaxDesiatov requested a review from AndrewHoos December 12, 2023 15:50

MaxDesiatov added the bug label Dec 12, 2023

AndrewHoos approved these changes Dec 12, 2023

View reviewed changes

MaxDesiatov added a commit that referenced this pull request Dec 12, 2023

Avoid using temp_await in SwiftTestTool.swift (#7016)

759174d

Follow up to #7009, which provides the detailed motivation: we should avoid `temp_await` as it can lead to deadlocks when combined with Swift Concurrency.

MaxDesiatov merged commit 6bed023 into main Dec 12, 2023

MaxDesiatov deleted the maxd/async-load-package branch December 12, 2023 19:14

MaxDesiatov added a commit that referenced this pull request Dec 13, 2023

Revert "Avoid using temp_await on loadRootPackage (#7009)"

a475c4e

This reverts commit 6bed023.

MaxDesiatov mentioned this pull request Dec 13, 2023

Revert "Avoid using temp_await on loadRootPackage" #7197

Closed

Avoid using temp_await on loadRootPackage #7009

Avoid using temp_await on loadRootPackage #7009

Uh oh!

Conversation

MaxDesiatov commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

MaxDesiatov Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 16, 2023

Uh oh!

MaxDesiatov commented Oct 17, 2023

Uh oh!

compnerd commented Oct 17, 2023

Uh oh!

MaxDesiatov commented Oct 17, 2023

Uh oh!

MaxDesiatov commented Dec 7, 2023

Uh oh!

MaxDesiatov commented Dec 7, 2023

Uh oh!

MaxDesiatov commented Dec 11, 2023

Uh oh!

MaxDesiatov commented Dec 12, 2023

Uh oh!

AndrewHoos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxDesiatov commented Dec 12, 2023

Uh oh!

MaxDesiatov commented Dec 12, 2023

Uh oh!

AndrewHoos Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewHoos Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewHoos Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Dec 12, 2023

Uh oh!

MaxDesiatov commented Dec 12, 2023

Uh oh!

Uh oh!

Avoid using `temp_await` on `loadRootPackage` #7009

Avoid using `temp_await` on `loadRootPackage` #7009

MaxDesiatov commented Oct 16, 2023 •

edited

Loading