From ec2ec31b5b2411432d815bee805a366e445b46c2 Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Thu, 27 Nov 2025 15:06:20 -0500 Subject: [PATCH 1/2] Fix concurrency warnings in AsyncProcess Squashes the warnings in AsyncProcess by adjusting required callbacks to be `@Sendable`, marking lock protected state as `nonisolated(unsafe)` and using a `ThreadSafeBox` to capture the process result. --- Sources/Basics/Concurrency/AsyncProcess.swift | 31 ++++++------ Sources/Commands/SwiftTestCommand.swift | 13 +++-- Tests/BasicsTests/AsyncProcessTests.swift | 47 ++++++++++++------- Tests/BasicsTests/CancellatorTests.swift | 2 +- Tests/CommandsTests/RunCommandTests.swift | 2 +- 5 files changed, 53 insertions(+), 42 deletions(-) diff --git a/Sources/Basics/Concurrency/AsyncProcess.swift b/Sources/Basics/Concurrency/AsyncProcess.swift index 17149e833bc..9cdca028170 100644 --- a/Sources/Basics/Concurrency/AsyncProcess.swift +++ b/Sources/Basics/Concurrency/AsyncProcess.swift @@ -257,26 +257,27 @@ package final class AsyncProcess { #endif /// Typealias for stdout/stderr output closure. - package typealias OutputClosure = ([UInt8]) -> Void + package typealias OutputClosure = @Sendable ([UInt8]) -> Void /// Typealias for logging handling closure - package typealias LoggingHandler = (String) -> Void + package typealias LoggingHandler = @Sendable (String) -> Void - private static var _loggingHandler: LoggingHandler? + nonisolated(unsafe) private static var _loggingHandler: LoggingHandler? private static let loggingHandlerLock = NSLock() /// Global logging handler. Use with care! preferably use instance level instead of setting one globally. - @available( - *, - deprecated, - message: "use instance level `loggingHandler` passed via `init` instead of setting one globally." - ) package static var loggingHandler: LoggingHandler? { get { Self.loggingHandlerLock.withLock { self._loggingHandler } - } set { + } + @available( + *, + deprecated, + message: "use instance level `loggingHandler` passed via `init` instead of setting one globally." + ) + set { Self.loggingHandlerLock.withLock { self._loggingHandler = newValue } @@ -330,7 +331,7 @@ package final class AsyncProcess { /// /// Key: Executable name or path. /// Value: Path to the executable, if found. - private static var validatedExecutablesMap = [String: AbsolutePath?]() + nonisolated(unsafe) private static var validatedExecutablesMap = [String: AbsolutePath?]() private static let validatedExecutablesMapLock = NSLock() /// Create a new process instance. @@ -812,17 +813,17 @@ package final class AsyncProcess { package func waitUntilExit() throws -> AsyncProcessResult { let group = DispatchGroup() group.enter() - var processResult: Result? + let resultBox = ThreadSafeBox>() self.waitUntilExit { result in - processResult = result + resultBox.put(result) group.leave() } group.wait() - return try processResult.unsafelyUnwrapped.get() + return try resultBox.get().unsafelyUnwrapped.get() } /// Executes the process I/O state machine, calling completion block when finished. - private func waitUntilExit(_ completion: @escaping (Result) -> Void) { + private func waitUntilExit(_ completion: @Sendable @escaping (Result) -> Void) { self.stateLock.lock() switch self.state { case .idle: @@ -1099,7 +1100,7 @@ extension AsyncProcess { environment: Environment = .current, loggingHandler: LoggingHandler? = .none, queue: DispatchQueue? = nil, - completion: @escaping (Result) -> Void + completion: @Sendable @escaping (Result) -> Void ) { let completionQueue = queue ?? Self.sharedCompletionQueue diff --git a/Sources/Commands/SwiftTestCommand.swift b/Sources/Commands/SwiftTestCommand.swift index 10c3b280c29..5c0a2cb9412 100644 --- a/Sources/Commands/SwiftTestCommand.swift +++ b/Sources/Commands/SwiftTestCommand.swift @@ -980,7 +980,7 @@ final class TestRunner { /// Executes and returns execution status. Prints test output on standard streams if requested /// - Returns: Result of spawning and running the test process, and the output stream result - func test(outputHandler: @escaping (String) -> Void) -> Result { + func test(outputHandler: @escaping @Sendable (String) -> Void) -> Result { var results = [Result]() for path in self.bundlePaths { let testSuccess = self.test(at: path, outputHandler: outputHandler) @@ -1027,11 +1027,11 @@ final class TestRunner { return args } - private func test(at path: AbsolutePath, outputHandler: @escaping (String) -> Void) -> Result { + private func test(at path: AbsolutePath, outputHandler: @escaping @Sendable (String) -> Void) -> Result { let testObservabilityScope = self.observabilityScope.makeChildScope(description: "running test at \(path)") do { - let outputHandler = { (bytes: [UInt8]) in + let outputHandler: @Sendable ([UInt8]) -> Void = { (bytes: [UInt8]) in if let output = String(bytes: bytes, encoding: .utf8) { outputHandler(output) } @@ -1214,17 +1214,16 @@ final class ParallelTestRunner { observabilityScope: self.observabilityScope, library: .xctest // swift-testing does not use ParallelTestRunner ) - var output = "" - let outputLock = NSLock() + let output = ThreadSafeBox("") let start = DispatchTime.now() - let result = testRunner.test(outputHandler: { _output in outputLock.withLock{ output += _output }}) + let result = testRunner.test(outputHandler: { _output in output.append(_output) }) let duration = start.distance(to: .now()) if result == .failure { self.ranSuccessfully = false } self.finishedTests.enqueue(TestResult( unitTest: test, - output: output, + output: output.get() ?? "", success: result != .failure, duration: duration )) diff --git a/Tests/BasicsTests/AsyncProcessTests.swift b/Tests/BasicsTests/AsyncProcessTests.swift index 656831ef3c2..e1113d3e2ab 100644 --- a/Tests/BasicsTests/AsyncProcessTests.swift +++ b/Tests/BasicsTests/AsyncProcessTests.swift @@ -76,15 +76,15 @@ final class AsyncProcessTests: XCTestCase { let args = ["whoami"] let answer = NSUserName() #endif - var popenResult: Result? + let popenResult = ThreadSafeBox>() let group = DispatchGroup() group.enter() AsyncProcess.popen(arguments: args) { result in - popenResult = result + popenResult.put(result) group.leave() } group.wait() - switch popenResult { + switch popenResult.get() { case .success(let processResult): let output = try processResult.utf8Output() XCTAssertTrue(output.hasPrefix(answer)) @@ -242,9 +242,11 @@ final class AsyncProcessTests: XCTestCase { } func testStdin() throws { - var stdout = [UInt8]() + let stdout = ThreadSafeBox<[UInt8]>([]) let process = AsyncProcess(scriptName: "in-to-out\(ProcessInfo.batSuffix)", outputRedirection: .stream(stdout: { stdoutBytes in - stdout += stdoutBytes + stdout.mutate { + $0?.append(contentsOf: stdoutBytes) + } }, stderr: { _ in })) let stdinStream = try process.launch() @@ -255,7 +257,7 @@ final class AsyncProcessTests: XCTestCase { try process.waitUntilExit() - XCTAssertEqual(String(decoding: stdout, as: UTF8.self), "hello\(ProcessInfo.EOL)") + XCTAssertEqual(String(decoding: stdout.get(default: []), as: UTF8.self), "hello\(ProcessInfo.EOL)") } func testStdoutStdErr() throws { @@ -352,28 +354,37 @@ final class AsyncProcessTests: XCTestCase { } func testStdoutStdErrStreaming() throws { - var stdout = [UInt8]() - var stderr = [UInt8]() + let stdout = ThreadSafeBox<[UInt8]>([]) + let stderr = ThreadSafeBox<[UInt8]>([]) let process = AsyncProcess(scriptName: "long-stdout-stderr\(ProcessInfo.batSuffix)", outputRedirection: .stream(stdout: { stdoutBytes in - stdout += stdoutBytes + stdout.mutate { + $0?.append(contentsOf: stdoutBytes) + } }, stderr: { stderrBytes in - stderr += stderrBytes + stderr.mutate { + $0?.append(contentsOf: stderrBytes) + } })) try process.launch() try process.waitUntilExit() let count = 16 * 1024 - XCTAssertEqual(String(bytes: stdout, encoding: .utf8), String(repeating: "1", count: count)) - XCTAssertEqual(String(bytes: stderr, encoding: .utf8), String(repeating: "2", count: count)) + XCTAssertEqual(String(bytes: stdout.get(default: []), encoding: .utf8), String(repeating: "1", count: count)) + XCTAssertEqual(String(bytes: stderr.get(default: []), encoding: .utf8), String(repeating: "2", count: count)) } func testStdoutStdErrStreamingRedirected() throws { - var stdout = [UInt8]() - var stderr = [UInt8]() + let stdout = ThreadSafeBox<[UInt8]>([]) + let stderr = ThreadSafeBox<[UInt8]>([]) + let process = AsyncProcess(scriptName: "long-stdout-stderr\(ProcessInfo.batSuffix)", outputRedirection: .stream(stdout: { stdoutBytes in - stdout += stdoutBytes + stdout.mutate { + $0?.append(contentsOf: stdoutBytes) + } }, stderr: { stderrBytes in - stderr += stderrBytes + stderr.mutate { + $0?.append(contentsOf: stderrBytes) + } }, redirectStderr: true)) try process.launch() try process.waitUntilExit() @@ -386,8 +397,8 @@ final class AsyncProcessTests: XCTestCase { let expectedStdout = String(repeating: "12", count: count) let expectedStderr = "" #endif - XCTAssertEqual(String(bytes: stdout, encoding: .utf8), expectedStdout) - XCTAssertEqual(String(bytes: stderr, encoding: .utf8), expectedStderr) + XCTAssertEqual(String(bytes: stdout.get(default: []), encoding: .utf8), expectedStdout) + XCTAssertEqual(String(bytes: stderr.get(default: []), encoding: .utf8), expectedStderr) } func testWorkingDirectory() throws { diff --git a/Tests/BasicsTests/CancellatorTests.swift b/Tests/BasicsTests/CancellatorTests.swift index e67030b0561..3433bfae498 100644 --- a/Tests/BasicsTests/CancellatorTests.swift +++ b/Tests/BasicsTests/CancellatorTests.swift @@ -451,7 +451,7 @@ class ProcessStartedSemaphore { self.term = term } - func handleOutput(_ bytes: [UInt8]) { + @Sendable func handleOutput(_ bytes: [UInt8]) { self.lock.withLock { guard !self.trapped else { return diff --git a/Tests/CommandsTests/RunCommandTests.swift b/Tests/CommandsTests/RunCommandTests.swift index 7da21d8fae7..2f239d24454 100644 --- a/Tests/CommandsTests/RunCommandTests.swift +++ b/Tests/CommandsTests/RunCommandTests.swift @@ -385,7 +385,7 @@ struct RunCommandTests { self.sync = sync } - func handle(bytes: [UInt8]) { + @Sendable func handle(bytes: [UInt8]) { guard let output = String(bytes: bytes, encoding: .utf8) else { return } From f42df929a80d86572a1784bb711c1bd4199e68c5 Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Mon, 1 Dec 2025 19:22:29 -0500 Subject: [PATCH 2/2] Use ThreadSafeBox/ThreadSafeKeyValueStore over nonisolated(unsafe) --- Sources/Basics/Concurrency/AsyncProcess.swift | 26 +++++-------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/Sources/Basics/Concurrency/AsyncProcess.swift b/Sources/Basics/Concurrency/AsyncProcess.swift index 9cdca028170..f4dedad5c48 100644 --- a/Sources/Basics/Concurrency/AsyncProcess.swift +++ b/Sources/Basics/Concurrency/AsyncProcess.swift @@ -259,18 +259,16 @@ package final class AsyncProcess { /// Typealias for stdout/stderr output closure. package typealias OutputClosure = @Sendable ([UInt8]) -> Void - /// Typealias for logging handling closure + /// Typealias for logging handling closure. package typealias LoggingHandler = @Sendable (String) -> Void - nonisolated(unsafe) private static var _loggingHandler: LoggingHandler? - private static let loggingHandlerLock = NSLock() + /// Global logging handler storage. + private static let _loggingHandler = ThreadSafeBox() /// Global logging handler. Use with care! preferably use instance level instead of setting one globally. package static var loggingHandler: LoggingHandler? { get { - Self.loggingHandlerLock.withLock { - self._loggingHandler - } + return _loggingHandler.get() ?? nil } @available( *, @@ -278,9 +276,7 @@ package final class AsyncProcess { message: "use instance level `loggingHandler` passed via `init` instead of setting one globally." ) set { - Self.loggingHandlerLock.withLock { - self._loggingHandler = newValue - } + _loggingHandler.put(newValue) } } @@ -331,8 +327,7 @@ package final class AsyncProcess { /// /// Key: Executable name or path. /// Value: Path to the executable, if found. - nonisolated(unsafe) private static var validatedExecutablesMap = [String: AbsolutePath?]() - private static let validatedExecutablesMapLock = NSLock() + private static let validatedExecutablesMap = ThreadSafeKeyValueStore() /// Create a new process instance. /// @@ -452,14 +447,7 @@ package final class AsyncProcess { } // This should cover the most common cases, i.e. when the cache is most helpful. if workingDirectory == localFileSystem.currentWorkingDirectory { - return AsyncProcess.validatedExecutablesMapLock.withLock { - if let value = AsyncProcess.validatedExecutablesMap[program] { - return value - } - let value = lookup() - AsyncProcess.validatedExecutablesMap[program] = value - return value - } + return AsyncProcess.validatedExecutablesMap.memoize(program, body: lookup) } else { return lookup() }