AsyncBufferSequence.Buffer Improvements #48

iCharlesHu · 2025-05-19T18:55:05Z

This PR enhances the usability of AsyncBufferSequence.Buffer and the closure-based run() in several ways:

Introduced AsyncBufferSequence.LineSequence, which lets developers stream standard output and error line by line as String. This is the recommended approach for converting AsyncBufferSequence.Buffer to String because converting each buffer individually might not always work due to potential breaks in the String.
Updated the implementation of AsyncBufferSequence.Buffer.bytes by basing it on DispatchData.Slice instead of DispatchData. This change ensures that AsyncBufferSequence.Buffer holds a contiguous chunk of bytes, which is necessary for the .bytes API.
Removed the unnecessary availability tags for the closure-based run().
Made several improvements to the documentation.

parkera · 2025-05-19T22:51:42Z

Sources/Subprocess/API.swift

@@ -267,7 +267,7 @@ public func run<Result, Error: OutputProtocol>(
    environment: Environment = .inherit,
    workingDirectory: FilePath? = nil,
    platformOptions: PlatformOptions = PlatformOptions(),
-    error: Error,
+    error: Error = .discarded,


Is this part of the same change?

Sources/Subprocess/Buffer.swift

jakepetroules · 2025-05-21T04:04:59Z

Sources/Subprocess/Buffer.swift

+                let span = RawSpan(_unsafeElements: ptr)
+                return _overrideLifetime(of: span, to: self)
+            case .array(let array):
+                let span = array.span.bytes


Swift 6.2 on Windows: error: value of type '[UInt8]' has no member 'span'

Hmm that's strange... should be available on 6.2. Let me check

iCharlesHu · 2025-05-21T04:11:33Z

In stead of directly expose String initializer, we decided to introduce AsyncBufferSequence.LineSequence which transforms AsyncBufferSequence to AsyncSequence<String> to allow line-by-line streaming

iCharlesHu · 2025-05-31T01:30:34Z

Sources/Subprocess/API.swift

 public func run<Result, Input: InputProtocol, Error: OutputProtocol>(
    _ executable: Executable,
    arguments: Arguments = [],
    environment: Environment = .inherit,
    workingDirectory: FilePath? = nil,
    platformOptions: PlatformOptions = PlatformOptions(),
    input: Input = .none,
-    error: Error,
+    error: Error = .discarded,


This was accidentally left out from the ~Copyable PR. I discovered it as I was testing closure based runs

Sources/Subprocess/API.swift

Sources/Subprocess/AsyncBufferSequence.swift

itingliu · 2025-06-02T23:26:33Z

Sources/Subprocess/AsyncBufferSequence.swift

-            return data
+            let createdBuffers = Buffer.createFrom(data)
+            // Most (all?) cases there should be only one buffer
+            // because DispatchData are motsly contiguous


Suggested change

// because DispatchData are motsly contiguous

// because DispatchData are mostly contiguous

itingliu · 2025-06-03T00:22:08Z

Sources/Subprocess/Buffer.swift

+        internal var startIndex: Int { self.bytes.startIndex }
+        internal var endIndex: Int { self.bytes.endIndex }
+
+        internal init(bytes: UnsafeBufferPointer<UInt8>) {


Is this missing a startIndex argument? without that,

internal var startIndex: Int { self.bytes.startIndex }

This will always return 0 since UnsafeBufferPointer.startIndex is always 0.

And without that it would not match the semantics of the other Slice-like types, where the index of the containing collection is preserved in the subsequence (like ArraySlice and Array) right?

Here we are intentionally keeping a 1 to 1 Buffer to Slice mapping. We actually don't really care the relative position of the Slices within the source DispatchData, as long as the slices are contiguous. This is the same conceptual design with the original Buffer to DispatchData mapping (as in we only care about individual chunk read). We only switched to slice instead of DispatchData because DispatchData is not contiguous and we need a contiguous storage in order to vend the bytes property.

Bikeshed: As we talked previously, since this is not really representing a sub-sequence like a Substring or ArraySlice, can we not call it Slice? It sounds like we're really interested in ensuring that it's contiguous, so perhaps ContiguousRegion or something like that?

Sources/Subprocess/Buffer.swift

…rom AsyncBufferSequence.Buffer

…closure based run()

parkera · 2025-06-05T19:41:52Z

Sources/Subprocess/AsyncBufferSequence.swift

+
+    public func lines<Encoding: _UnicodeEncoding>(
+        encoding: Encoding.Type = UTF8.self,
+        bufferingPolicy: LineSequence<Encoding>.BufferingPolicy = .unbounded


Maybe we should label all 'new' API since 0.0.1 (including the ones I worked on) with some kind of comment so we know which ones we need to have further discussion about in review. Here I would like to talk about the tradeoff when defaulting to an unbounded buffer, with no arguments...

Currently I track them by tagging the PRs with API Change label. I'll add some comments and when we are ready for 0.0.1 I'll list out all the changes.

I chose .unbounded as the default argument because that's what AsyncStream do today.

If we decide to throw an error for #45
can we please have an issue tracking updating this to the same behavior?

Sources/Subprocess/AsyncBufferSequence.swift

Sources/Subprocess/IO/Output.swift

Sources/Subprocess/Teardown.swift

iCharlesHu · 2025-06-06T00:00:29Z

Addressed Tony's comments. Most noticeably: throw an error when LineSequence buffered enough content that exceeds the maxLineLength limit.

iCharlesHu · 2025-06-06T00:03:24Z

Resolves #15

parkera · 2025-06-09T19:53:23Z

Sources/Subprocess/AsyncBufferSequence.swift

+                    if self.buffer.isEmpty {
+                        return nil
+                    }
+                    return String(decoding: self.buffer[0 ..< endIndex], as: Encoding.self)


I'm still a little unclear on why we need to copy this into an Array, and then copy it into a String. Why can't String read from the buffer data directly?

We have to keep an [UInt8] buffer in order to scan through and find new line characters. We can't use String directly as the buffer type because every time when we receive a chunk of data, we can't directly convert it to String since the chunk might end in between byte encodings.

parkera · 2025-06-09T22:35:55Z

Sources/Subprocess/AsyncBufferSequence.swift

@@ -61,7 +59,15 @@ public struct AsyncBufferSequence: AsyncSequence, Sendable {
                #endif
                return nil
            }
-            return data
+            let createdBuffers = Buffer.createFrom(data)


Style nitpick, but this is just init.

Or, in this case it sort of goes the other way since it vends buffers from data... I guess we can clean it up later.

And also it creates multiple. I didn't use initializer because it's DispatchData -> [Buffer].

Tests/SubprocessTests/SubprocessTests+Unix.swift

itingliu · 2025-06-09T22:34:53Z

Sources/Subprocess/Buffer.swift

+        internal var startIndex: Int { self.bytes.startIndex }
+        internal var endIndex: Int { self.bytes.endIndex }
+
+        internal init(bytes: UnsafeBufferPointer<UInt8>) {


Bikeshed: As we talked previously, since this is not really representing a sub-sequence like a Substring or ArraySlice, can we not call it Slice? It sounds like we're really interested in ensuring that it's contiguous, so perhaps ContiguousRegion or something like that?

itingliu · 2025-06-09T22:37:26Z

Sources/Subprocess/Buffer.swift

+#if canImport(Darwin) || canImport(Glibc) || canImport(Android) || canImport(Musl)
+extension DispatchData {
+    /// Unfortunately `DispatchData.Region` is not available on Linux, hence our own wrapper
+    internal struct Slice: @unchecked Sendable, RandomAccessCollection {


Why is this @unchecked Sendable?

Because it holds a let bytes: UnsafeBufferPointer<UInt8> property. DispatchData.Region's backing storage is also @unchecked Sendable, I suspect for the same reason.

Sources/Subprocess/Buffer.swift

iCharlesHu · 2025-06-09T23:23:11Z

Bikeshed: As we talked previously, since this is not really representing a sub-sequence like a Substring or ArraySlice, can we not call it Slice? It sounds like we're really interested in ensuring that it's contiguous, so perhaps ContiguousRegion or something like that?

How about _ContiguousBufferView? Which follows Swift's *View naming convention that it's a "view" into a larger data. I'll also add underscore to make it clear it's internal and only used by Subprocess.

…turning partial (and potentially incorrect) data

iCharlesHu requested a review from parkera May 19, 2025 18:55

iCharlesHu marked this pull request as ready for review May 19, 2025 18:55

iCharlesHu force-pushed the charles/buffer-improvements branch from 36e0e21 to f261182 Compare May 19, 2025 21:29

iCharlesHu changed the title ~~Add .stringValue(), .arrayValue(), and .dataValue() to Buffer~~ Introduce custom initializers to initialize String, Data, and Array from AsyncBufferSequence.Buffer May 19, 2025

iCharlesHu force-pushed the charles/buffer-improvements branch from f261182 to 75b804c Compare May 19, 2025 22:38

iCharlesHu added the API Change label May 19, 2025

parkera reviewed May 19, 2025

View reviewed changes

glessard reviewed May 20, 2025

View reviewed changes

Sources/Subprocess/Buffer.swift Outdated Show resolved Hide resolved

jakepetroules reviewed May 21, 2025

View reviewed changes

iCharlesHu marked this pull request as draft May 21, 2025 04:09

iCharlesHu changed the title ~~Introduce custom initializers to initialize String, Data, and Array from AsyncBufferSequence.Buffer~~ AsyncBufferSequence.Buffer Improvements May 30, 2025

iCharlesHu marked this pull request as ready for review May 30, 2025 19:27

iCharlesHu requested a review from itingliu May 30, 2025 19:28

iCharlesHu commented May 31, 2025

View reviewed changes

itingliu reviewed Jun 3, 2025

View reviewed changes

iCharlesHu and others added 6 commits June 5, 2025 10:21

Introduce custom initializers to initialize String, Data, and Array f…

0712fdb

…rom AsyncBufferSequence.Buffer

Update Sources/Subprocess/Buffer.swift

a774271

Implement LineSequence

c3e85bb

Update Buffer to be based on DispatchData.Slice

b864ce4

Remove unnecessary availability check around AsyncBufferSequence and …

d37e9f2

…closure based run()

Add documentation for the new closure based run()

07953c2

iCharlesHu force-pushed the charles/buffer-improvements branch from be5121b to 8fa05f4 Compare June 5, 2025 17:29

Fix typos througout the repo

00bcd99

iCharlesHu force-pushed the charles/buffer-improvements branch from 8fa05f4 to 00bcd99 Compare June 5, 2025 18:30

parkera reviewed Jun 5, 2025

View reviewed changes

Sources/Subprocess/AsyncBufferSequence.swift Outdated Show resolved Hide resolved

parkera reviewed Jun 5, 2025

View reviewed changes

Sources/Subprocess/AsyncBufferSequence.swift Outdated Show resolved Hide resolved

parkera reviewed Jun 5, 2025

View reviewed changes

Sources/Subprocess/AsyncBufferSequence.swift Outdated Show resolved Hide resolved

parkera reviewed Jun 5, 2025

View reviewed changes

Sources/Subprocess/IO/Output.swift Outdated Show resolved Hide resolved

parkera reviewed Jun 5, 2025

View reviewed changes

Sources/Subprocess/Teardown.swift Outdated Show resolved Hide resolved

iCharlesHu mentioned this pull request Jun 6, 2025

Fix sample usage of closure #53

Closed

iCharlesHu mentioned this pull request Jun 6, 2025

Run with Custom Closure sample code not compiling #15

Closed

iCharlesHu force-pushed the charles/buffer-improvements branch from b192b17 to ddeb6d0 Compare June 6, 2025 04:32

parkera reviewed Jun 9, 2025

View reviewed changes

Tests/SubprocessTests/SubprocessTests+Unix.swift Outdated Show resolved Hide resolved

parkera approved these changes Jun 9, 2025

View reviewed changes

itingliu reviewed Jun 9, 2025

View reviewed changes

LineSequence: throw error when max line length exceeded instead of re…

195fcee

…turning partial (and potentially incorrect) data

iCharlesHu force-pushed the charles/buffer-improvements branch from ddeb6d0 to 195fcee Compare June 10, 2025 00:21

iCharlesHu merged commit 8a8c6b7 into swiftlang:main Jun 10, 2025
20 checks passed

	// because DispatchData are motsly contiguous
	// because DispatchData are mostly contiguous

AsyncBufferSequence.Buffer Improvements #48

AsyncBufferSequence.Buffer Improvements #48

Uh oh!

Conversation

iCharlesHu commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iCharlesHu commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itingliu Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iCharlesHu commented Jun 6, 2025

Uh oh!

iCharlesHu commented Jun 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iCharlesHu Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iCharlesHu commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

iCharlesHu commented May 19, 2025 •

edited

Loading

itingliu Jun 3, 2025 •

edited

Loading

iCharlesHu Jun 9, 2025 •

edited

Loading