Skip to content

Remove temp_await [part 1] #7761

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
Aug 1, 2024
Merged

Remove temp_await [part 1] #7761

merged 1 commit into from
Aug 1, 2024

Conversation

AndrewHoos
Copy link
Contributor

@AndrewHoos AndrewHoos commented Jul 5, 2024

Remove both temp_await and safe_async in favor of broader adoption of async/await

Note:

This change was originally developed as one large PR. To make testing, reviewing, and landing these changes simpler the PR has been broken down into about a dozen individual PRs that are stacked

Motivation:

temp_await is very dangerous API that blocks the current thread and is not marked as unavailable from async methods. The only need for temp_await is because LLBuild only supports non-async methods so any llbuild commands that call async methods have to block waiting.

Because temp_await is liberally spread across the code base there are a number of async methods that may indirectly call into temp_await and indefinitely block a swift concurrency thread. This risks a deadlock. To prevent the deadlock the safe_async method dispatches to a GCD concurrent queue before executing the callback code. Because GCD will create new threads if all existing threads are blocked (unlike swift concurrency) this effectively prevents deadlock risks at the risk of thread explosion.

It would be better if the codebase was free of both the risks of deadlock and thread explosion. Additionally it would be nice if the APIs using temp_await were marked async so that more parts of the code base could safely adopt async/await.

Modifications:

To augment existing memoizer types new async types are added
temp_dir is replaced by unsafe_await which is limited to one used which is called by llbuild
safe_async is replaced by CheckedThrowingContinuation
Multiple protocols have been modified to change their requirements from sync to async
Usage of DispatchSemaphore and DispatchGroup have been replaced with structured concurrency
Many methods that were non-async but used temp_await have been properly marked as async

Result:

No usage of temp_dir or safe_async and only one (safe) usage of unsafe_await from an LLBuild command

@MaxDesiatov
Copy link
Contributor

@swift-ci test

@AndrewHoos AndrewHoos force-pushed the deprecateTempAwait branch from 40046ad to e928031 Compare July 8, 2024 19:10
@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos AndrewHoos force-pushed the deprecateTempAwait branch from e928031 to 6ef615f Compare July 8, 2024 21:44
@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos AndrewHoos changed the title Deprecate temp_await and being moving to async/await Deprecate temp_await and begin moving to async/await Jul 9, 2024
@AndrewHoos
Copy link
Contributor Author

@swift-ci test windows

let workspace = try swiftCommandState.getActiveWorkspace()
let root = try swiftCommandState.getWorkspaceRoot()
let rootManifests = try temp_await {
let rootManifests = try await safe_async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern here is the same as on previous PRs: safe_async uses DispatchQueue.sharedConcurrent under the hood, which can cause thread explosion. IMO instead of using that we need a serial queue for manifest loading, or OperationQueue with a fixed number of threads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this can just be a checked continuation, no? Why does it even need to move off the caller at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that the workspace make use of temp_await which blocks threads so if we call into it from a swift concurrency pool we run the risk of blocking a thread. I know that GCD risks thread explosion, but that is less bad than a deadlocked thread IMO. At the root of this tension is the integration into the LLBuild system which does not support async/await or callbacks. I think I have a plan for how I can limit the unsafe blocking behavior to GCD queue dedicated to builds. Let me take some time to revisit this and make it more holistically address this

@AndrewHoos AndrewHoos force-pushed the deprecateTempAwait branch from 6ef615f to 80ee0e9 Compare July 14, 2024 06:28
@AndrewHoos AndrewHoos changed the title Deprecate temp_await and begin moving to async/await Remove temp_await and safe_async throughout the entire codebase Jul 14, 2024
@AndrewHoos
Copy link
Contributor Author

@swift-ci test

3 similar comments
@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

I don't understand the compiler error from macOS. The body of with*Continuation executes serially on the actor so it should be safe to access isolated state

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos AndrewHoos changed the title Remove temp_await and safe_async throughout the entire codebase Remove temp_await and safe_async throughout the entire codebase [part 1] Aug 1, 2024
@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

@AndrewHoos
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, the remaining uses of safe_async are isolated to CLI commands, which should be acceptable for now. Thanks!

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) August 1, 2024 19:30
@MaxDesiatov MaxDesiatov changed the title Remove temp_await and safe_async throughout the entire codebase [part 1] Remove temp_await in parts of the codebase Aug 1, 2024
@AndrewHoos AndrewHoos changed the title Remove temp_await in parts of the codebase Remove temp_await [part 1] Aug 1, 2024
@MaxDesiatov MaxDesiatov merged commit 1764337 into main Aug 1, 2024
5 checks passed
@MaxDesiatov MaxDesiatov deleted the deprecateTempAwait branch August 1, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants