-
Notifications
You must be signed in to change notification settings - Fork 16
Adopt ~Copyable in Subprocess
#38
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
base: main
Are you sure you want to change the base?
Adopt ~Copyable in Subprocess
#38
Conversation
821723e
to
2754c5f
Compare
2754c5f
to
f126559
Compare
var inputIOBox: TrackedPlatformDiskIO? = consume inputIO | ||
var outputIOBox: TrackedPlatformDiskIO? = consume outputIO | ||
var errorIOBox: TrackedPlatformDiskIO? = consume errorIO | ||
return try await withThrowingTaskGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a task group, how about using async let
? It's shorter, simpler, and takes better advantage of the type system (no force unwraps or OutputCapturingState helper type needed):
...
) { execution, inputIO, outputIO, errorIO in
// Write input, capture output and error in parallel
var inputIOBox: TrackedPlatformDiskIO? = consume inputIO
var outputIOBox: TrackedPlatformDiskIO? = consume outputIO
var errorIOBox: TrackedPlatformDiskIO? = consume errorIO
var outputIOContainer: TrackedPlatformDiskIO? = outputIOBox.take()
var errorIOContainer: TrackedPlatformDiskIO? = errorIOBox.take()
async let stdout = output.captureOutput(
from: outputIOContainer.take()
)
async let stderror = error.captureOutput(
from: errorIOContainer.take()
)
// Write span at the same isolation
if let writeFd = inputIOBox.take() {
let writer = StandardInputWriter(diskIO: writeFd)
_ = try await writer.write(input._bytes)
try await writer.finish()
}
return try await (
processIdentifier: execution.processIdentifier,
standardOutput: stdout,
standardError: stderror
)
}
...
// more probable. We use `preconditionFailure` upon receiving `.badFileDescriptor` | ||
// to prevent accidentally closing a different file descriptor. | ||
guard errno != .badFileDescriptor else { | ||
preconditionFailure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be fatalError
? The string of preconditionFailure
is not retained in release builds, which will make the cause of crashes here, harder to identify.
// Input | ||
var result: Int32 = -1 | ||
if let inputRead = inputPipe.readFileDescriptor { | ||
result = posix_spawn_file_actions_adddup2(&fileActions, inputRead.platformDescriptor, 0) | ||
if inputReadFileDescriptor != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest using if let inputReadFileDescriptor {
so you don't need the force unwrap below.
This PR introduces
~Copyable
intoSubprocess
, includingCreatedPipe
,TrackedFileDescriptor
, andTrackedDispatchIO
.Resolves #37
Resolves #2
Additionally, we opted to slightly modify the API by moving the
.standardOutput
and.standardError
AsyncSequence
fromExecution
to the closure parameter, alongsideExecution
. This adjustment eliminated the necessity forAtomic
(andAtomicBox
) withinExecution
.Furthermore, this change rendered
SequenceOutput
internal, as we now rely on overloads to infer the output and error types.