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:

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

Finally, add a new public init to HTTPClientResponse:

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

Result

Users can now freely initialise and modify HTTPClientReponses:

varresponse=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

-publicfunc makeAsyncIterator()->AsyncIterator{
-AsyncIterator(stream:IteratorStream(bag:self.bag))
+
+@inlinablepublicinit(){


Suggested change
@inlinablepublicinit(){
@inlinablepublicinit(){

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.
+publicstructBody:AsyncSequence,Sendable{
+publictypealiasElement=ByteBuffer
+@usableFromInlinetypealiasStorage=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

+}
+}
+
+@inlinablefunc 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
privatefunc 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:

extensionHTTPClientRequest.Body{publicstaticfunc bytes(_ byteBuffer:ByteBuffer)->Self public static func bytes<Bytes: RandomAccessCollection &Sendable>( _ bytes:Bytes)->Selfwhere Bytes.Element ==UInt8{Self._bytes(bytes)}publicstaticfunc bytes<Bytes:Sequence&Sendable>( _ bytes:Bytes, length:Length)->Selfwhere Bytes.Element ==UInt8 public static func bytes<Bytes: Collection &Sendable>( _ bytes: Bytes, length: Length )-> Self where Bytes.Element == UInt8publicstaticfunc stream<SequenceOfBytes:AsyncSequence&Sendable>( _ sequenceOfBytes:SequenceOfBytes, length:Length)->Selfwhere 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,*)
+extensionSequence{
+ /// Turns `self` into an `AsyncSequence` by vending each element of `self` asynchronously.
+@inlinablevarasync: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

+}
+}
+
+@inlinablefunc 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 1f146a4Compare 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 7dfed60Compare 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 Adds new public API. 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:mainOct 11, 2022

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

	privatefunc 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

Uh oh!

Conversation

dnadoba commented Oct 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Result

Uh oh!

Uh oh!

Uh oh!

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!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

dnadoba commented Oct 5, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make `HTTPClientResponse.init` public#632

Make `HTTPClientResponse.init` public #632

dnadoba commented Oct 4, 2022•
edited
Loading