Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iCharlesHu
Copy link
Contributor

@iCharlesHu iCharlesHu commented May 7, 2025

This PR introduces ~Copyable into Subprocess, including CreatedPipe, TrackedFileDescriptor, and TrackedDispatchIO.

Resolves #37
Resolves #2

Additionally, we opted to slightly modify the API by moving the .standardOutput and .standardError AsyncSequence from Execution to the closure parameter, alongside Execution. This adjustment eliminated the necessity for Atomic (and AtomicBox) within Execution.

Furthermore, this change rendered SequenceOutput internal, as we now rely on overloads to infer the output and error types.

@iCharlesHu iCharlesHu force-pushed the charles/noncopyable-execution branch from 2754c5f to f126559 Compare May 9, 2025 19:22
var inputIOBox: TrackedPlatformDiskIO? = consume inputIO
var outputIOBox: TrackedPlatformDiskIO? = consume outputIO
var errorIOBox: TrackedPlatformDiskIO? = consume errorIO
return try await withThrowingTaskGroup(
Copy link
Contributor

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(
Copy link
Contributor

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 {
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants