[WIP] Async body currency type #46
Conversation
| import struct Foundation.Data // only for convenience initializers | ||
|
|
||
| /// The type representing a request or response body. | ||
| public final class Body: @unchecked Sendable { |
There was a problem hiding this comment.
Why is this a class? Also can we get rid of the @unchecked Sendable here and rather make ourselves a LockedValueBox abstraction that is conditionally Sendable when its contents are. This way we are making sure we really are Sendable
There was a problem hiding this comment.
Why is this a class?
Because the body is inherently a reference type that cannot be freely copied.
Also can we get rid of the @unchecked Sendable here and rather make ourselves a LockedValueBox abstraction that is conditionally Sendable when its contents are. This way we are making sure we really are Sendable.
Sure, that's an option that we can take as well, fortunately that won't affect the public API, so we can do that at any time.
| public typealias DataType = ArraySlice<UInt8> | ||
|
|
||
| /// How many times the provided sequence can be iterated. | ||
| public enum IterationBehavior: Sendable { |
There was a problem hiding this comment.
This probably should be a struct.
There was a problem hiding this comment.
Because API evolution? If so, I'd happily mark this as @frozen or give some other hint that we won't be expanding the enum.
| /// | ||
| /// Supports retries and redirects, as a new iterator is created each | ||
| /// time. | ||
| case multiple |
There was a problem hiding this comment.
Are we ever expecting an AsyncSequence based body to be multiple here? Right now that just doesn't exist in the ecosystem.
There was a problem hiding this comment.
Yes, for example any body created from a sync collection can be iterated multiple times. An async sequence for reading a file could also be legal to iterate multiple times. Basically, we want to provide the tools for those who want to support e.g. retryable file uploads.
| public let iterationBehavior: IterationBehavior | ||
|
|
||
| /// The total length of the body, if known. | ||
| public enum Length: Sendable { |
There was a problem hiding this comment.
Because API evolution? If so, I'd happily mark this as @frozen or give some other hint that we won't be expanding the enum.
| } | ||
| } | ||
|
|
||
| extension Body: Equatable { |
There was a problem hiding this comment.
I am not sure if we really need Equatable and Hashable on the body
There was a problem hiding this comment.
We do, as this type will be owned by generated Equatable/Hashable types.
| public convenience init( | ||
| stream: AsyncThrowingStream<some StringProtocol, any Error>, | ||
| length: Body.Length | ||
| ) { | ||
| self.init( | ||
| sequence: .init(stream.map(\.asBodyChunk)), | ||
| length: length, | ||
| iterationBehavior: .single | ||
| ) | ||
| } | ||
|
|
||
| public convenience init( | ||
| stream: AsyncStream<some StringProtocol>, | ||
| length: Body.Length | ||
| ) { | ||
| self.init( | ||
| sequence: .init(stream.map(\.asBodyChunk)), | ||
| length: length, | ||
| iterationBehavior: .single | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same answer - iterationBehavior: .single is why this initializer is helpful.
| /// Accumulates the full body in-memory into a single buffer | ||
| /// up to `maxBytes`, converts it to Foundation.Data, and returns it. | ||
| /// - Parameters: | ||
| /// - maxBytes: The maximum number of bytes this method is allowed | ||
| /// to accumulate in memory before it throws an error. | ||
| /// - Throws: `TooManyBytesError` if the the body contains more | ||
| /// than `maxBytes`. | ||
| public func collectAsData(upTo maxBytes: Int) async throws -> Data { | ||
| let bytes: DataType = try await collect(upTo: maxBytes) | ||
| return Data(bytes) | ||
| } |
There was a problem hiding this comment.
IMO this doesn't carry its weight for me since it is a one-liner for the user.
There was a problem hiding this comment.
It was meant to mirror collectAsString in a way, and tries to help with systems where Data is the very common data type (iOS apps). Plus, it'll be called from generated code, from where we really try to only make simple calls, as opposed to embedding too much logic.
| self.produceNext = { | ||
| try await iterator.next() | ||
| } |
There was a problem hiding this comment.
Why are we not storing the iterator here but a closure?
There was a problem hiding this comment.
The intent here is type-erasure, is there a better way than to go through a closure?
There was a problem hiding this comment.
Even if AsyncIteratorProtocol had primary associated type, we would prefer a closure as it will get specialised but an existential not, as of today.
| typealias AsyncIterator = Body.Iterator | ||
| typealias Element = DataType | ||
|
|
||
| private let produceIterator: () -> AsyncIterator |
| } | ||
|
|
||
| /// A wrapper for a sync sequence. | ||
| private struct WrappedSyncSequence<S: Sequence>: AsyncSequence |
There was a problem hiding this comment.
FWIW we have this in async-algorithms.
| public func mapChunks( | ||
| _ transform: @escaping @Sendable (Element) async -> Element | ||
| ) -> HTTPBody { | ||
| let validatedTransform: @Sendable (Element) async -> Element | ||
| switch length { | ||
| case .known: | ||
| validatedTransform = { element in | ||
| let transformedElement = await transform(element) | ||
| guard transformedElement.count == element.count else { | ||
| fatalError( | ||
| "OpenAPIRuntime.HTTPBody.mapChunks transform closure attempted to change the length of a chunk in a body which has a total length specified, this is not allowed." | ||
| ) | ||
| } | ||
| return transformedElement | ||
| } | ||
| case .unknown: | ||
| validatedTransform = transform | ||
| } | ||
| return HTTPBody( | ||
| sequence: map(validatedTransform), | ||
| length: length, | ||
| iterationBehavior: iterationBehavior | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the use case for this method?
There was a problem hiding this comment.
All these use cases (streaming encryption/unencryption/zipping/unzipping) will not work as they will all change the chunk length. These are also stateful operations and the @Sendable requirement will make this needlessly hard. In addition they will usually buffer data and will need an explicit flush at the end which is not possible with this API. These use cases should just be modelled as AsyncSequence operators/transformers.
There was a problem hiding this comment.
Yeah I can be convinced, I'll remove the API, it's not pulling its weight.
| // MARK: - String-based bodies | ||
|
|
||
| extension StringProtocol { | ||
| fileprivate var asBodyChunk: HTTPBody.DataType { |
There was a problem hiding this comment.
@inlinable or otherwise quite slow as this is a generic over StringProtocol
There was a problem hiding this comment.
This is an internal symbol, I thought the compiler doesn't need inlinable annotations for internal symbols, only public ones? (It should be inlining these even without the annotation I thought.)
There was a problem hiding this comment.
You call this in a public generic function (HTTPBody.init(data: some StringProtocol, ...)) which forwards the generic type to this function call. Therefore it needs to be @inlinable or it can't be specialised.
There was a problem hiding this comment.
Would @_usableFromInline be enough here?
There was a problem hiding this comment.
No, it will allow it to be called form @inlinable but not allow for specialisation. This means you also need to make it internal which sucks but that's how it is.
You can also read more about it in the proposal but please fell free to continue asking me directly :)
Everything that's generic and public, right? |
|
And internal functions if they are called from public generic functions and forward the generic type to internal functions. |
|
@dnadoba @FranzBusch Let's move the conversation to the PR that has this plus the new currency types, as that's where I put more usage examples: https://github.com/apple/swift-openapi-runtime/pull/47/files#diff-e089a09ba06f85ed1377a6d7ec2a0c36a9f91a3d1b276134aa3156ec6484cede |
Motivation
An early prototype of #9.
Modifications
TODO
Result
TODO
Test Plan
TODO