Adopt ~Copyable in Subprocess
#38
Conversation
iCharlesHu
force-pushed
the
charles/noncopyable-execution
branch
3 times, most recently
from
821723e to
2754c5f
Compare
iCharlesHu
force-pushed
the
charles/noncopyable-execution
branch
from
2754c5f to
f126559
Compare
| 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.
Suggest using if let inputReadFileDescriptor { so you don't need the force unwrap below.
There was a problem hiding this comment.
Unfortunately this pattern, along with if let a = a is not compatible with ~Copyable types. Writing if let inputReadFileDescriptor would have consumed inputReadFileDescriptor so the line below would be double consume.
iCharlesHu
force-pushed
the
charles/noncopyable-execution
branch
from
f126559 to
2d28695
Compare
iCharlesHu
force-pushed
the
charles/noncopyable-execution
branch
from
2d28695 to
7c35edb
Compare
|
Addressed review comments and removed |
| cleanupHandler: { error in | ||
| // Close the file descriptor | ||
| if self.closeWhenDone { | ||
| try? self.safelyClose() |
There was a problem hiding this comment.
why ignore the error? That can't be quite right
| "FileDescriptor \(fileDescriptor.rawValue) is already closed" | ||
| ) | ||
| } | ||
| // Otherwise ignore the error |
There was a problem hiding this comment.
this is a bad idea. Closing in deinit for file descriptors is bad enough (close may block too depending on the fd) but ignoring the errors isn't a thing we can do.
There was a problem hiding this comment.
I agree with @weissi here FDs cannot be modeled with ~Copyable types at this point both due the async close nature and the errors on close. My recommendation is to use with-style methods which allow to solve both issues.
There was a problem hiding this comment.
Yup, agree with Franz. with* solves those issues correctly and also clearly for a reader of the code.
There was a problem hiding this comment.
@weissi can you open up issues for your comments? This PR is merged and discussions here might be lost.
|
|
||
| internal var platformDescriptor: PlatformFileDescriptor { | ||
| deinit { | ||
| guard self.closeWhenDone else { |
There was a problem hiding this comment.
This looks like we're hiding some lifecycle bugs here, no? We should know if we closed a file descriptor or not, right?
6 participants
This PR introduces
~CopyableintoSubprocess, includingCreatedPipe,TrackedFileDescriptor, andTrackedDispatchIO.Resolves #37
Resolves #2
Additionally, we opted to slightly modify the API by moving the
.standardOutputand.standardErrorAsyncSequencefromExecutionto the closure parameter, alongsideExecution. This adjustment eliminated the necessity forAtomic(andAtomicBox) withinExecution.Furthermore, this change rendered
SequenceOutputinternal, as we now rely on overloads to infer the output and error types.This patch also drops support for
Swift 6.0because it can't properly support the~Copyablework. NowSwift 6.1is the minimal required Swift version to buildSubprocess