-
Notifications
You must be signed in to change notification settings - Fork 17
Allow callers to run a subprocess and provide low and high water marks when using SequenceOutput to emit standard output and standard error as soon as it arrives. #40
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?
Changes from 2 commits
7324ac4
7b6899c
9b173ab
ab43ae4
10aa523
aa903ad
bb57bf2
955e9c9
5f2df5f
4bac06a
ae0de61
d65f2ce
c8fe778
4de3d79
30876f5
ff7ae12
48b97f2
d94a9c6
888666b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,8 +127,8 @@ extension Configuration { | |
output: output, | ||
error: error, | ||
inputPipe: inputPipe.createInputPipe(), | ||
outputPipe: outputPipe.createOutputPipe(), | ||
errorPipe: errorPipe.createOutputPipe() | ||
outputPipe: outputPipe.createOutputPipe(with: platformOptions.outputOptions), | ||
errorPipe: errorPipe.createOutputPipe(with: platformOptions.errorOptions) | ||
) | ||
} | ||
|
||
|
@@ -176,6 +176,8 @@ public struct PlatformOptions: Sendable { | |
// Creates a session and sets the process group ID | ||
// i.e. Detach from the terminal. | ||
public var createSession: Bool = false | ||
public var outputOptions: StreamOptions = .init() | ||
public var errorOptions: StreamOptions = .init() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we only need one (see my comments below). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I replaced these with |
||
/// An ordered list of steps in order to tear down the child | ||
/// process in case the parent task is cancelled before | ||
/// the child proces terminates. | ||
|
@@ -197,6 +199,18 @@ public struct PlatformOptions: Sendable { | |
public init() {} | ||
} | ||
|
||
extension PlatformOptions { | ||
public struct StreamOptions: Sendable { | ||
let lowWater: Int? | ||
let highWater: Int? | ||
|
||
init(lowWater: Int? = nil, highWater: Int? = nil) { | ||
self.lowWater = lowWater | ||
self.highWater = highWater | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I know I initially suggested using a How about we try something like this instead? struct PlatformOptions {
…
let preferredStreamBufferSizeRange: Range<Int>? = nil
} This approach makes it clear that we’re requesting a range (with a lower and upper bound), and it eliminates the need for validation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu The issue I see with this suggestion is that it presumes that you will set both the lower and upper bound, or neither. There is no mechanism for setting just one or the other. As I see it, we have a few options:
Thoughts? I think option 3 is too awkward and unintuitive, so I don't think we should consider it. If you feel strongly about option 1, we can go with that, but I wanted to bring up this issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu I modified your proposal a bit to use a Check it out and let me know what you think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ahh I see your point. In this case let's go back to
I think that's the simplest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu Ok, great. I've gone back to configuring this through |
||
} | ||
} | ||
|
||
extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible { | ||
internal func description(withIndent indent: Int) -> String { | ||
let indent = String(repeating: " ", count: indent * 4) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -665,6 +665,48 @@ extension SubprocessUnixTests { | |
#expect(catResult.terminationStatus.isSuccess) | ||
#expect(catResult.standardError == expected) | ||
} | ||
|
||
@Test func testSlowDripRedirectedOutputRedirectToSequence() async throws { | ||
let threshold: Double = 0.5 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately in tests you'll have to write
In the beginning to work around the same availability issue. See other tests for examples. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu When I first started working on this, I was very confused as to why some of the tests weren't running my new code and it was because of this check. Wouldn't it be better to have them skipped and noted as such in the test output rather than falsely succeeding? I'm thinking something like this:
Of course, we can have a helper function to make this less verbose. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu I went ahead and conditionalized this one test this way as an example. Let me know if you don't like that and would like me to revert to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rdingman unfortunately this won't work because on Swift 6.2 and above the compiler will complain the code inside of Unfortunately so far this is the only way I found that works. Very ugly... but it'll have to do for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iCharlesHu Hmm. This builds just fine for me with the latest Swift 6.2 toolchain, has the intended results, and works with the currently shipping Swift 6.1 toolchain. However, I'll go ahead and revert this and match the existing tests. |
||
|
||
let script = """ | ||
echo "DONE" | ||
sleep \(threshold) | ||
""" | ||
|
||
var platformOptions = PlatformOptions() | ||
platformOptions.outputOptions = .init(lowWater: 0) | ||
|
||
let start = ContinuousClock().now | ||
|
||
let catResult = try await Subprocess.run( | ||
.path("/bin/bash"), | ||
arguments: ["-c", script], | ||
platformOptions: platformOptions, | ||
output: .sequence, | ||
error: .discarded, | ||
body: { (execution, _) in | ||
for try await chunk in execution.standardOutput { | ||
let string = chunk.withUnsafeBytes { String(decoding: $0, as: UTF8.self) } | ||
|
||
if string.hasPrefix("DONE") { | ||
let end = ContinuousClock().now | ||
|
||
if (end - start) > .seconds(threshold) { | ||
return "Failure" | ||
|
||
} else { | ||
return "Success" | ||
} | ||
} | ||
} | ||
|
||
return "Failure" | ||
} | ||
) | ||
#expect(catResult.terminationStatus.isSuccess) | ||
#expect(catResult.value == "Success") | ||
} | ||
} | ||
|
||
// MARK: - PlatformOption Tests | ||
|
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.
This does not seem right. Why are we creating a new iterator when the first one ends? There will be nothing to read from this second iterator because all the data in the pipe would already been read.
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.
Thanks for the pushback on this. I was going to explain my thinking and then realize if I have to explain this then it probably should be written in a more straightforward manner. The first implementation was using one iterator per chunk and when we reached a chunk boundary we'd switch to a new iterator. While this did work, it was admittedly a little clunky. Now, we use one AsyncThrowingStream (and iterator) across all chunks (even if the chunk is broken up into sub-chunks fora single read as in my original motivation for this issue. Please check on the new implementation to see if it makes sense to you.