Conditionally adopt swift-subprocess #638
Conversation
jakepetroules
requested review from
aciidgh,
mirza-garibovic,
mhrawdon,
neonichu and
owenv
as code owners
|
@swift-ci test |
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
from
7e290ca to
5d1747c
Compare
|
@swift-ci test |
| platformOptions.teardownSequence = [.gracefulShutDown(allowedDurationToNextStep: .seconds(5))] | ||
| #if os(macOS) | ||
| if highPriority { | ||
| platformOptions.qualityOfService = .userInitiated |
There was a problem hiding this comment.
@compnerd Would it seem reasonable to call SetPriorityClass with HIGH_PRIORITY_CLASS for Windows (REALTIME_PRIORITY_CLASS definitely sounded excessive)? Currently this is used to elevate the build service process so that it can serve high-priority requests more efficiently (e.g. with Swift Concurrency task priority set higher than normal).
There was a problem hiding this comment.
Hmm, we should try it - it can saturate the system to the point of being unusable though, so I am a little concerned about that.
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
from
5d1747c to
1d4c11c
Compare
|
@swift-ci test |
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
2 times, most recently
from
922c246 to
bd4c32d
Compare
Sources/SWBUtil/Process.swift
Outdated
| platformOptions.teardownSequence = [.gracefulShutDown(allowedDurationToNextStep: .seconds(5))] | ||
| } | ||
| let result = try await Subprocess.run(.path(FilePath(url.filePath.str)), arguments: .init(arguments), environment: environment.map { .custom(.init($0)) } ?? .inherit, workingDirectory: (currentDirectoryURL?.filePath.str).map { FilePath($0) } ?? nil, platformOptions: platformOptions, body: { execution, inputWriter, outputReader, errorReader in | ||
| try await inputWriter.finish() |
There was a problem hiding this comment.
potential deadlock here. You can't assume that you can fully unload the stdin w/o reading stdout/err
There was a problem hiding this comment.
There's no deadlock here, all this is doing is closing a file descriptor. We have no input. That said, I'll move it below the output reading just to be correct.
| } | ||
| let result = try await Subprocess.run(.path(FilePath(url.filePath.str)), arguments: .init(arguments), environment: environment.map { .custom(.init($0)) } ?? .inherit, workingDirectory: (currentDirectoryURL?.filePath.str).map { FilePath($0) } ?? nil, platformOptions: platformOptions, body: { execution, inputWriter, outputReader, errorReader in | ||
| try await inputWriter.finish() | ||
| async let stdoutBytes = outputReader.collect().flatMap { $0.withUnsafeBytes(Array.init) } |
There was a problem hiding this comment.
so? we can't use Span yet, this still has to build with 6.1
| let result = try await Subprocess.run(.path(FilePath(url.filePath.str)), arguments: .init(arguments), environment: environment.map { .custom(.init($0)) } ?? .inherit, workingDirectory: (currentDirectoryURL?.filePath.str).map { FilePath($0) } ?? nil, platformOptions: platformOptions, body: { execution, inputWriter, outputReader, errorReader in | ||
| try await inputWriter.finish() | ||
| async let stdoutBytes = outputReader.collect().flatMap { $0.withUnsafeBytes(Array.init) } | ||
| async let stderrBytes = errorReader.collect().flatMap { $0.withUnsafeBytes(Array.init) } |
There was a problem hiding this comment.
collect will load all of this potentially infinite stream into memory and I don't even see a limit, seems dangerous. If anything, limit it to something high like 32 MB and throw if we're blowing through that.
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
3 times, most recently
from
89d45c3 to
f84bfcf
Compare
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
from
f84bfcf to
5cd9b6f
Compare
This fully resolves working directory thread safety issues with subprocess spawning across all platforms. For now, subprocess is adopted conditionally in order to continue building in certain environments where the Subprocess module may not be available, in which case we fall back to Foundation Process. Closes swiftlang#441
jakepetroules
force-pushed
the
eng/PR-swift-subprocess
branch
from
5cd9b6f to
ec02157
Compare
This fully resolves working directory thread safety issues with subprocess spawning across all platforms. For now, subprocess is adopted conditionally in order to continue building in certain environments where the Subprocess module may not be available, in which case we fall back to Foundation Process.
Closes #441
(migrated from #584 in accordance with our new policy to require PRs from forks)