Skip to content

Fix a minor threading issue and TODO by switching to structured concurrency #2245

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.

Sign up for GitHub

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
Oct 4, 2023
Merged

Fix a minor threading issue and TODO by switching to structured concurrency #2245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 50 additions & 45 deletions CodeGeneration/Sources/generate-swift-syntax/GenerateSwiftSyntax.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ struct TemplateSpec {
}

@main
struct GenerateSwiftSyntax: ParsableCommand {
struct GenerateSwiftSyntax: AsyncParsableCommand {
@Argument(help: "The path to the source directory (i.e. 'swift-syntax/Sources') where the source files are to be generated")
var destination: String = {
let sourcesURL = URL(fileURLWithPath: #filePath)
Expand All @@ -83,7 +83,7 @@ struct GenerateSwiftSyntax: ParsableCommand {
@Flag(help: "Enable verbose output")
var verbose: Bool = false

func run() throws {
func run() async throws {
let destination = URL(fileURLWithPath: self.destination).standardizedFileURL

var fileSpecs: [GeneratedFileSpec] = [
Expand Down Expand Up @@ -139,7 +139,6 @@ struct GenerateSwiftSyntax: ParsableCommand {

let modules = Set(fileSpecs.compactMap { $0.pathComponents.first })

let previouslyGeneratedFilesLock = NSLock()
var previouslyGeneratedFiles = Set(
modules.flatMap { (module) -> [URL] in
let generatedDir =
Expand All @@ -154,32 +153,37 @@ struct GenerateSwiftSyntax: ParsableCommand {
}
)

var errors: [Error] = []
DispatchQueue.concurrentPerform(iterations: fileSpecs.count) { index in
let fileSpec = fileSpecs[index]
do {
var destination = destination
for component in fileSpec.pathComponents {
destination = destination.appendingPathComponent(component)
await withTaskGroup(of: (url: URL, error: Error?).self) { group in
for fileSpec in fileSpecs {
group.addTask {
do {
var destination = destination
for component in fileSpec.pathComponents {
destination = destination.appendingPathComponent(component)
}
do {
try generateFile(
contents: fileSpec.contents,
destination: destination,
verbose: verbose
)
} catch {
// If we throw from here, we'll lose the URL,
// and we'll end up removing a file that is still
// included in the files we intend to generate,
// even if we failed to do so on this run.
return (destination, error)
}
return (destination, nil)
}
}
}
for await result in group {
_ = previouslyGeneratedFiles.remove(result.url)
if let error = result.error {
print("Error when generating file at \(result.url):", error)
}

previouslyGeneratedFilesLock.lock();
_ = previouslyGeneratedFiles.remove(destination)
previouslyGeneratedFilesLock.unlock()

try generateFile(
contents: fileSpec.contents,
destination: destination,
verbose: verbose
)
} catch {
errors.append(error)
}
}

if let firstError = errors.first {
// TODO: It would be nice if we could emit all errors
throw firstError
}

for file in previouslyGeneratedFiles {
Expand All @@ -189,24 +193,25 @@ struct GenerateSwiftSyntax: ParsableCommand {
}
}

private func generateFile(
contents: @autoclosure () -> String,
destination: URL,
verbose: Bool
) throws {
try FileManager.default.createDirectory(
atPath: destination.deletingLastPathComponent().path,
withIntermediateDirectories: true,
attributes: nil
)
}

if verbose {
print("Generating \(destination.path)...")
}
let start = Date()
try contents().write(to: destination, atomically: true, encoding: .utf8)
if verbose {
print("Generated \(destination.path) in \((Date().timeIntervalSince(start) * 1000).rounded() / 1000)s")
}
fileprivate func generateFile(
contents: @autoclosure () -> String,
destination: URL,
verbose: Bool
) throws {
Comment on lines +198 to +202
Copy link
Member

Choose a reason for hiding this comment

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

I’m happy with making this a top-level function but just FYI, you could keep this a member of GenerateSwiftSyntax if you mark the function as nonisolated.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't actually use any members, so IMO a top level fileprivate function makes sense anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out you don't have to even mark it as nonisolated in the current setup of run, it still works.

I originally made this change because when I tried to mark run as async without changing the conformance to the async version of ParseableCommand, the compiler error kind of implied this was the problem. That was not the case.

So I don't have a strong opinion on this, but I'm partial to the top level free function.

try FileManager.default.createDirectory(
atPath: destination.deletingLastPathComponent().path,
withIntermediateDirectories: true,
attributes: nil
)

if verbose {
print("Generating \(destination.path)...")
}
let start = Date()
try contents().write(to: destination, atomically: true, encoding: .utf8)
if verbose {
print("Generated \(destination.path) in \((Date().timeIntervalSince(start) * 1000).rounded() / 1000)s")
}
}