Make `HTTPClientResponse.init` public #632

dnadoba · 2022-10-04T23:51:05Z

Motivation

We want to allow user to initialise HTTPClientResponse for mocking purposes in tests.

Modifications

add AnyAsyncSequence<Element> to erase the type of the user provided AsyncSequences. We need this because we do not want to make HTTPClientResponse.Body generic over the specific AsyncSequence.
rename the old implementation of HTTPClientResponse.Body to TransactionBody and make it internal.
remove no longer necessary type hiding in TransactionBody as it is internal.
add Either<A, B> and conditionally conform it to AsyncSequence if A and B are are AsyncSequences with the same Element type. We use it to still allow the compiler to specialize the HTTPClientResponse.Body default case where the AsyncSequence is of type TransactionBody
use Either<TransactionBody, AnyAsyncSeqence<ByteBuffer> as the new storage type for HTTPClientReponse.Body. We still hide the concrete storage type behind a struct.
move AsyncSequenceFromSyncSequence from AsyncHTTPClientTests to AsyncHTTPClient and make it inlinable. (we could replace the implementation with AsyncLazySequence from swift-async-algorithms)
Add there new public HTTPClientResponse.Body inits:

extension HTTPClientResponse.Body { 
    init() { ... }
    static func bytes(_ byteBuffer: ByteBuffer) { ... }
    static func stream<SequenceOfBytes>(
        _ sequenceOfBytes: SequenceOfBytes
    ) where SequenceOfBytes: AsyncSequence & Sendable, SequenceOfBytes.Element == ByteBuffer { ... }
}

Finally, add a new public init to HTTPClientResponse:

extension HTTPClientReponse {
    init(
        version: HTTPVersion = .http1_1,
        status: HTTPResponseStatus = .ok,
        headers: HTTPHeaders = [:],
        body: Body = Body()
    ) { ... }
}

Result

Users can now freely initialise and modify HTTPClientReponses:

var response = HTTPClientResponse()
response.headers = [
    "Content-Type": "text/html"
]
response.body = .bytes(ByteBuffer(string: "<html> .... </html>"))

Sources/AsyncHTTPClient/AsyncAwait/AnyAsyncSequence.swift

Sources/AsyncHTTPClient/AsyncAwait/AsyncSequenceFromSyncSequence.swift

Lukasa · 2022-10-05T07:14:50Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

-    public func makeAsyncIterator() -> AsyncIterator {
-        AsyncIterator(stream: IteratorStream(bag: self.bag))
+
+    @inlinable public init(){


Suggested change

@inlinable public init(){

@inlinable public init() {

Is there any reason not to make this a memberwise initializer with these defaults already provided?

We eventually want to add support for trailing headers and this would require us to add another init for it if we want to allow trailing headers in the init too.
However, I'm fine with adding a member wise init already today if you are too. We aren't likely to add any more properties to this struct (famous last words) and one additional init in the future isn't too bad.

Lukasa · 2022-10-05T07:16:56Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+    /// are entirely synthetic and have no semantic meaning.
+    public struct Body: AsyncSequence, Sendable {
+        public typealias Element = ByteBuffer
+        @usableFromInline typealias Storage = Either<TransactionBody, AnyAsyncSequence<ByteBuffer>>


TBH, I think the use of Either is excessively clever, and it leads to some unclear code (case a and case b). I'd rather see a specific-case enum for this use-case.

FranzBusch

In general, this looks like a fine approach to me. However, I would like to discuss the similarities and differences between the approach we took in gRPC and here. In gRPC we are just providing a testing interface which you can use to drive the sequence. Here we allow the user to provider their own AsyncSequence.

The former makes it slightly easier to get started testing since you can just use that interface to yield and finish. The latter provides a bit more customisation because you can drive more than just the next method, i.e. also the makeAsyncIterator.

Sources/AsyncHTTPClient/AsyncAwait/AsyncSequenceFromSyncSequence.swift

FranzBusch · 2022-10-05T09:58:24Z

Sources/AsyncHTTPClient/AsyncAwait/TransactionBody.swift

+        }
+    }
+
+    @inlinable func makeAsyncIterator() -> AsyncIterator {


On a different note and from our other learnings, we might want to ensure that only a single iterator is ever created here.

We do something similar already here:

async-http-client/Sources/AsyncHTTPClient/AsyncAwait/Transaction+StateMachine.swift

Lines 641 to 653 in f17a47e

private func verifyStreamIDIsEqual(

registered: HTTPClientResponse.Body.IteratorStream.ID,

this: HTTPClientResponse.Body.IteratorStream.ID,

file: StaticString = #file,

line: UInt = #line

) {

if registered != this {

preconditionFailure(

"Tried to use a second iterator on response body stream. Multiple iterators are not supported.",

file: file, line: line

)

}

}

This implementation is a bit more clever. It will allow the creation of multiple iterators and will only fail slightly delayed if someone tries to iterate over more than one of them. I think this is actually too clever and we should remove this eventually and just fail early during the creation of a second iterator. I'm not aware of any use case for creating unused iterators but maybe @fabianfett has some in mind.

Nope, it was the best thing I came up with at the time. I def. like the atomic approach that we can see here the most so far. SingleIteratorPrecondition. However I'm not sure if we should change this behavior now. Because this would IMO be a breaking change. I just went through this in GRPC...

I'm inclined to agree with Fabian: there's no good sense in breaking the use-case that already exists, even though it'd be better defined if we did.

Changed it back to the old behaviour. However, we now only enforce this on user provided AsyncSequences. Can also change that to use the more relaxed precondition if you want.

dnadoba · 2022-10-05T11:53:41Z

I think that in AHC we usually do not want to write individual ByteBuffers to the stream. My guess is that the most common use case will be to just init the Body with a complete ByteBuffer.

We may also want to think about aligning the Request and Response API in AHC.
A HTTPClientRequest.Body can be created in the following ways:

extension HTTPClientRequest.Body {
    public static func bytes(_ byteBuffer: ByteBuffer) -> Self

    public static func bytes<Bytes: RandomAccessCollection & Sendable>(
        _ bytes: Bytes
    ) -> Self where Bytes.Element == UInt8 {
        Self._bytes(bytes)
    }
    public static func bytes<Bytes: Sequence & Sendable>(
        _ bytes: Bytes,
        length: Length
    ) -> Self where Bytes.Element == UInt8

    public static func bytes<Bytes: Collection & Sendable>(
        _ bytes: Bytes,
        length: Length
    ) -> Self where Bytes.Element == UInt8

    public static func stream<SequenceOfBytes: AsyncSequence & Sendable>(
        _ sequenceOfBytes: SequenceOfBytes,
        length: Length
    ) -> Self where SequenceOfBytes.Element == ByteBuffer
    
    public static func stream<Bytes: AsyncSequence & Sendable>(
        _ bytes: Bytes,
        length: Length
    ) -> Self where Bytes.Element == UInt8
}

We could provide a similar interface for HTTPClientResponse.Body excluding the length parameter.

fabianfett · 2022-10-10T14:19:57Z

Sources/AsyncHTTPClient/AsyncAwait/AsyncSequenceFromSyncSequence.swift

+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+extension Sequence {
+    /// Turns `self` into an `AsyncSequence` by vending each element of `self` asynchronously.
+    @inlinable var async: AsyncLazySequence<Self> {


This modifier already exists, doesn't it? I see it is local here, but I assume that we want to make it public eventually...

We don't want to make it public. It exists in swift-async-algorithms. However, it doesn't yet have a 1.x release.

Lukasa · 2022-10-10T16:28:27Z

Sources/AsyncHTTPClient/AsyncAwait/SingleIteratorPrecondition.swift

+
+import Atomics
+
+/// Makes sure that a consumer of this `AsyncSequence`


Make sure they what?

Lukasa · 2022-10-10T16:30:24Z

Sources/AsyncHTTPClient/AsyncAwait/TransactionBody.swift

+        }
+    }
+
+    @inlinable func makeAsyncIterator() -> AsyncIterator {


I'm inclined to agree with Fabian: there's no good sense in breaking the use-case that already exists, even though it'd be better defined if we did.

dnadoba marked this pull request as draft October 5, 2022 00:15

dnadoba force-pushed the dn-response-init branch from 84dde48 to 1f146a4 Compare October 5, 2022 00:28

Lukasa requested a review from FranzBusch October 5, 2022 07:12

Lukasa reviewed Oct 5, 2022

View reviewed changes

FranzBusch reviewed Oct 5, 2022

View reviewed changes

dnadoba added 3 commits October 10, 2022 14:59

Make HTTPClientResponse.init public

bbbda70

Align AsyncSequenceFromSyncSequence with swift-async-algorithms

13e03cd

Address review comments

7dfed60

dnadoba force-pushed the dn-response-init branch from d5da21d to 7dfed60 Compare October 10, 2022 13:59

SwiftFormat

c2ffb93

dnadoba marked this pull request as ready for review October 10, 2022 14:01

dnadoba requested review from FranzBusch and Lukasa October 10, 2022 14:01

fabianfett reviewed Oct 10, 2022

View reviewed changes

Rename file to match type name

09a5977

Lukasa added the semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Oct 10, 2022

Lukasa reviewed Oct 10, 2022

View reviewed changes

Fix review comments

c04f621

dnadoba requested a review from Lukasa October 11, 2022 08:57

Lukasa approved these changes Oct 11, 2022

View reviewed changes

FranzBusch approved these changes Oct 11, 2022

View reviewed changes

dnadoba merged commit d7b69d9 into swift-server:main Oct 11, 2022

dnadoba deleted the dn-response-init branch October 11, 2022 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `HTTPClientResponse.init` public #632

Make `HTTPClientResponse.init` public #632

dnadoba commented Oct 4, 2022 •

edited

Loading

Lukasa Oct 5, 2022

Lukasa Oct 5, 2022

dnadoba Oct 5, 2022

Lukasa Oct 5, 2022

FranzBusch left a comment

FranzBusch Oct 5, 2022

dnadoba Oct 10, 2022

fabianfett Oct 10, 2022

Lukasa Oct 10, 2022

dnadoba Oct 11, 2022

dnadoba commented Oct 5, 2022

fabianfett Oct 10, 2022

dnadoba Oct 10, 2022

Lukasa Oct 10, 2022

Lukasa Oct 10, 2022

	private func verifyStreamIDIsEqual(
	registered: HTTPClientResponse.Body.IteratorStream.ID,
	this: HTTPClientResponse.Body.IteratorStream.ID,
	file: StaticString = #file,
	line: UInt = #line
	) {
	if registered != this {
	preconditionFailure(
	"Tried to use a second iterator on response body stream. Multiple iterators are not supported.",
	file: file, line: line
	)
	}
	}


		import Atomics

		/// Makes sure that a consumer of this `AsyncSequence`

Make HTTPClientResponse.init public #632

Make HTTPClientResponse.init public #632

Conversation

dnadoba commented Oct 4, 2022 • edited Loading

Motivation

Modifications

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadoba commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `HTTPClientResponse.init` public #632

Make `HTTPClientResponse.init` public #632

dnadoba commented Oct 4, 2022 •

edited

Loading