Collect function fix #672

carolinacass · 2023-03-16T10:56:28Z

Motivation:
not accumulate too many bytes

Modifications:
Implementing collect function to use NIOCore version to prevent overflowing

Motivation: not accumulate too many bytes Modifications: Implementing collect function to use NIOCore version to prevent overflowing

dnadoba · 2023-03-16T11:04:22Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+        /// - Throws: `NIOTooManyBytesError` if the the sequence contains more than `maxBytes`.
+        /// - Returns: the number of bytes collected over time
+        func collect(maxBytes: Int) async throws -> ByteBuffer {
+            return try await self.collect(upTo: maxBytes)


If the response head already announces the body size we can check that size before downloading anything. This is e.g. the case if the response contains a Content-Length header which contains the expected body size. If it is bigger than maxBytes we should throw before calling self.collection(upTo:). To actually do that we need to move this extension from HTTPClientResponse.Body to HTTPClientResponse or have some kind of reference in HTTPClientResponse.Body to the headers.

Be careful though, there are edge cases where we don't expect a body even if Content-Length is set. This is e.g. the case if the request was just a HEAD request. I think there are more edge cases but that's the only one I know of the top of my head.

I do not think this is appropriate for helper functions to check the Content-Length, but will confirm that it deals with it correctly.

Lukasa · 2023-03-16T13:50:38Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

@@ -83,6 +83,15 @@ extension HTTPClientResponse {
        @inlinable public func makeAsyncIterator() -> AsyncIterator {
            .init(storage: self.storage.makeAsyncIterator())
        }
+
+        /// Accumulates an ``Swift/AsyncSequence`` of ``ByteBuffer``s into a single ``ByteBuffer``.


We can be clearer here: this accumulates Body.

Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift

dnadoba

Great, a couple of small tweaks and we should be good to go.

dnadoba · 2023-03-22T16:00:24Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

@@ -32,16 +32,32 @@ public struct HTTPClientResponse: Sendable {
    /// The body of this HTTP response.
    public var body: Body

+    @usableFromInline internal var requestMethod: HTTPMethod?


we don't need the requestMethod any more as we compute the expextedContentLength in the init.

dnadoba · 2023-03-22T16:01:20Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+                    print(maxBytes)
+                    print("contentLength", contentLength)


Suggested change

print(maxBytes)

print("contentLength", contentLength)

dnadoba · 2023-03-22T16:01:49Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+
+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+extension HTTPClientResponse {
+//    need to pass in the arguments


Suggested change

// need to pass in the arguments

dnadoba · 2023-03-22T16:02:29Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+extension HTTPClientResponse {
+//    need to pass in the arguments
+    static func expectedContentLength(requestMethod: HTTPMethod?, headers: HTTPHeaders, status: HTTPResponseStatus) -> Int? {


Suggested change

static func expectedContentLength(requestMethod: HTTPMethod?, headers: HTTPHeaders, status: HTTPResponseStatus) -> Int? {

static func expectedContentLength(requestMethod: HTTPMethod, headers: HTTPHeaders, status: HTTPResponseStatus) -> Int? {

We make request method non-optional now

dnadoba · 2023-03-22T16:03:34Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+        let contentLengths = headers["content-length"].first.flatMap({Int($0, radix: 10)})
+        return contentLengths


Suggested change

let contentLengths = headers["content-length"].first.flatMap({Int($0, radix: 10)})

return contentLengths

let contentLength = headers["content-length"].first.flatMap({Int($0, radix: 10)})

return contentLength

dnadoba · 2023-03-22T16:06:20Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

@@ -146,7 +201,7 @@ extension HTTPClientResponse.Body {
    @inlinable public static func stream<SequenceOfBytes>(
        _ sequenceOfBytes: SequenceOfBytes
    ) -> Self where SequenceOfBytes: AsyncSequence & Sendable, SequenceOfBytes.Element == ByteBuffer {
-        self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)))
+        self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)), expectedContentLength: nil)


Suggested change

self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)), expectedContentLength: nil)

self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)))

dnadoba · 2023-03-22T16:07:51Z

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift

+    func testReponseInitWithStatus() {
+        guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
+        XCTAsyncTest {
+            var response = HTTPClientResponse(status: .notModified , requestMethod: .GET)
+            response.headers.replaceOrAdd(name: "content-length", value: "1025")
+            guard let body = await XCTAssertNoThrowWithResult(
+                try await response.body.collect(upTo: 1024)
+            ) else { return }
+            XCTAssertEqual(0, body.readableBytes)
+        }
+    }


This is no longer testing what we want. I think we can just remove it

dnadoba · 2023-03-22T16:08:10Z

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift

+private func makeDefaultHTTPClient(
+    eventLoopGroupProvider: HTTPClient.EventLoopGroupProvider = .createNew
+) -> HTTPClient {
+    var config = HTTPClient.Configuration()
+    config.tlsConfiguration = .clientDefault
+    config.tlsConfiguration?.certificateVerification = .none
+    config.httpVersion = .automatic
+    return HTTPClient(
+        eventLoopGroupProvider: eventLoopGroupProvider,
+        configuration: config,
+        backgroundActivityLogger: Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+    )
+}


After we have removed the last test case, this is no longer used

dnadoba · 2023-03-22T16:08:48Z

Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift

+            response.headers.replaceOrAdd(name: "content-length", value: "1025")
+            XCTAssertEqual(response.headers["content-length"], ["1025"])


This is not affecting the test and we can remove it

dnadoba · 2023-03-22T16:08:55Z

Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift

+            let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+            var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/")
+            request.method = .GET
+            request.body = .bytes(ByteBuffer(string: "1235"))


Nit:

Suggested change

request.body = .bytes(ByteBuffer(string: "1235"))

request.body = .bytes(ByteBuffer(string: "1234"))

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

dnadoba · 2023-03-23T10:07:08Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+        }
+        else {


Suggested change

}

else {

} else {

dnadoba · 2023-03-23T10:07:57Z

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift

+private func makeDefaultHTTPClient(
+    eventLoopGroupProvider: HTTPClient.EventLoopGroupProvider = .createNew
+) -> HTTPClient {
+    var config = HTTPClient.Configuration()
+    config.tlsConfiguration = .clientDefault
+    config.tlsConfiguration?.certificateVerification = .none
+    config.httpVersion = .automatic
+    return HTTPClient(
+        eventLoopGroupProvider: eventLoopGroupProvider,
+        configuration: config,
+        backgroundActivityLogger: Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+    )
+}


dnadoba · 2023-03-23T10:08:01Z

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift

+    func testReponseInitWithStatus() {
+        guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { return }
+        XCTAsyncTest {
+            var response = HTTPClientResponse(status: .notModified , requestMethod: .GET)
+            response.headers.replaceOrAdd(name: "content-length", value: "1025")
+            guard let body = await XCTAssertNoThrowWithResult(
+                try await response.body.collect(upTo: 1024)
+            ) else { return }
+            XCTAssertEqual(0, body.readableBytes)
+        }
+    }


dnadoba · 2023-03-23T10:11:57Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift

+        status: HTTPResponseStatus = .ok,
+        headers: HTTPHeaders = [:],
+        body: Body = Body(),
+        requestMethod: HTTPMethod?


We can remove the requestMethod argument name again

dnadoba · 2023-03-23T10:47:50Z

Sources/AsyncHTTPClient/AsyncAwait/HTTPClientResponse.swift


-    @inlinable public init(
+    @inlinable init(


This init needs to stay public

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift

Sources/AsyncHTTPClient/AsyncAwait/TransactionBody.swift

Lukasa · 2023-03-24T08:10:11Z

Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift

+            let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:))
+            var request = HTTPClientRequest(url: "https://localhost:\(bin.port)/")
+            request.method = .GET
+            request.body = .bytes(ByteBuffer(string: "1234"))


Hrm, I think this muddies the waters of the test somewhat. I don't think this test would fail if you took away the content-length checks, because the actual collect call will still end up violating the content-length.

What you want is for the HTTPbin server to send you back a content-length that's too long and no body at all. That should still throw this error.

Lukasa · 2023-03-30T07:17:33Z

@swift-server-bot add to allowlist

Lukasa

Nice one, this LGTM.

Lukasa · 2023-03-30T07:20:34Z

Ah, sorry, can you run generate_linux_tests.rb please?

Lukasa · 2023-03-30T11:31:32Z

You'll also need to run ./scripts/soundness.sh.

carolinacass added 3 commits March 16, 2023 10:55

Collect function fix

668b33a

Motivation: not accumulate too many bytes Modifications: Implementing collect function to use NIOCore version to prevent overflowing

minor change

6276e51

Merge branch 'main' into cc-collectResponse

104eff5

dnadoba requested changes Mar 16, 2023

View reviewed changes

Lukasa requested changes Mar 16, 2023

View reviewed changes

carolinacass added 7 commits March 16, 2023 15:54

putting back in tests accidentally deleted

2ad4a6b

Fixing comment

e919181

incorrect ordering

df627a5

Main to this file

8156844

Checking the Content Length through requestMethod and status

9b63815

minor move

e8d8f85

Changing placement and fixing tests to expectecdContentLength

0de9e27

dnadoba requested changes Mar 22, 2023

View reviewed changes

carolinacass added 2 commits March 23, 2023 10:03

suggested fixes

045a063

fixing comments and deleting excess functions

9dc6f33

dnadoba reviewed Mar 23, 2023

View reviewed changes

getting rid of request Method

ecbcd90

dnadoba reviewed Mar 23, 2023

View reviewed changes

Tests/AsyncHTTPClientTests/HTTPClientResponseTests.swift Show resolved Hide resolved

adding tests for expectedContentLength and public init

ae17940

dnadoba approved these changes Mar 23, 2023

View reviewed changes

dnadoba requested a review from Lukasa March 23, 2023 11:25

Lukasa added the 🆕 semver/minor Adds new public API. label Mar 24, 2023

Lukasa requested changes Mar 24, 2023

View reviewed changes

Fixing test and spelling

483a3f2

carolinacass requested a review from Lukasa March 29, 2023 15:33

Lukasa approved these changes Mar 30, 2023

View reviewed changes

running generate_linux_tests.rb

14cfdd4

fixing formatting

e60d65b

Lukasa mentioned this pull request Apr 3, 2023

Accept bare 2023 in license header #676

Merged

Merge branch 'main' into cc-collectResponse

2fbef23

Lukasa approved these changes Apr 4, 2023

View reviewed changes

Lukasa merged commit 91b2640 into swift-server:main Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect function fix #672

Collect function fix #672

carolinacass commented Mar 16, 2023

dnadoba Mar 16, 2023 •

edited

Loading

carolinacass Mar 20, 2023

Lukasa Mar 16, 2023

dnadoba left a comment

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 23, 2023

dnadoba Mar 22, 2023

dnadoba Mar 23, 2023

dnadoba Mar 22, 2023

dnadoba Mar 22, 2023

dnadoba Mar 23, 2023

dnadoba Mar 23, 2023

dnadoba Mar 23, 2023

dnadoba Mar 23, 2023

dnadoba Mar 23, 2023

Lukasa Mar 24, 2023

Lukasa commented Mar 30, 2023

Lukasa left a comment

Lukasa commented Mar 30, 2023

Lukasa commented Mar 30, 2023

	static func expectedContentLength(requestMethod: HTTPMethod?, headers: HTTPHeaders, status: HTTPResponseStatus) -> Int? {
	static func expectedContentLength(requestMethod: HTTPMethod, headers: HTTPHeaders, status: HTTPResponseStatus) -> Int? {

		let contentLengths = headers["content-length"].first.flatMap({Int($0, radix: 10)})
		return contentLengths

	self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)), expectedContentLength: nil)
	self.init(.anyAsyncSequence(AnyAsyncSequence(sequenceOfBytes.singleIteratorPrecondition)))

		response.headers.replaceOrAdd(name: "content-length", value: "1025")
		XCTAssertEqual(response.headers["content-length"], ["1025"])

	request.body = .bytes(ByteBuffer(string: "1235"))
	request.body = .bytes(ByteBuffer(string: "1234"))

Collect function fix #672

Collect function fix #672

Conversation

carolinacass commented Mar 16, 2023

dnadoba Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadoba 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

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

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

Choose a reason for hiding this comment

Lukasa commented Mar 30, 2023

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented Mar 30, 2023

Lukasa commented Mar 30, 2023

dnadoba Mar 16, 2023 •

edited

Loading