Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.x.x - Trailer headers #330

Merged
merged 6 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Benchmarks/Benchmarks/Router/RouterBenchmarks.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ extension Benchmark {
let hbRequest = HBRequest(head: request, body: requestBody)
group.addTask {
let response = try await responder.respond(to: hbRequest, context: context)
try await response.body.write(BenchmarkBodyWriter())
_ = try await response.body.write(BenchmarkBodyWriter())
}
try await writeBody(requestBodyStream)
requestBodyStream.finish()
Expand Down Expand Up @@ -126,6 +126,7 @@ func routerBenchmarks() {
for try await buffer in request.body {
try await writer.write(buffer)
}
return nil
})
}
}
Expand Down
33 changes: 30 additions & 3 deletions Sources/HummingbirdCore/Response/ResponseBody.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//
//===----------------------------------------------------------------------===//

import HTTPTypes
import NIOCore

public protocol HBResponseBodyWriter {
Expand All @@ -20,15 +21,15 @@ public protocol HBResponseBodyWriter {

/// Response body
public struct HBResponseBody: Sendable {
public let write: @Sendable (any HBResponseBodyWriter) async throws -> Void
public let write: @Sendable (any HBResponseBodyWriter) async throws -> HTTPFields?
public let contentLength: Int?

/// Initialise HBResponseBody with closure writing body contents
/// - Parameters:
/// - contentLength: Optional length of body
/// - write: closure provided with `writer` type that can be used to write to response body
public init(contentLength: Int? = nil, _ write: @Sendable @escaping (any HBResponseBodyWriter) async throws -> Void) {
self.write = write
self.write = { try await write($0); return nil }
self.contentLength = contentLength
}

Expand All @@ -40,7 +41,9 @@ public struct HBResponseBody: Sendable {
/// Initialise HBResponseBody that contains a single ByteBuffer
/// - Parameter byteBuffer: ByteBuffer to write
public init(byteBuffer: ByteBuffer) {
self.init(contentLength: byteBuffer.readableBytes) { writer in try await writer.write(byteBuffer) }
self.init(contentLength: byteBuffer.readableBytes) { writer in
try await writer.write(byteBuffer)
}
}

/// Initialise HBResponseBody with an AsyncSequence of ByteBuffers
Expand All @@ -50,6 +53,30 @@ public struct HBResponseBody: Sendable {
for try await buffer in asyncSequence {
try await writer.write(buffer)
}
return
}
}

/// Create HBResponseBody that returns trailing headers from its closure once all the
/// body parts have been written
/// - Parameters:
/// - contentLength: Optional length of body
/// - write: closure provided with `writer` type that can be used to write to response body
/// trailing headers are returned from the closure after all the body parts have been
/// written
public static func withTrailingHeaders(
contentLength: Int? = nil,
_ write: @Sendable @escaping (any HBResponseBodyWriter) async throws -> HTTPFields?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot better, now one more idea to play with: Can we make the any HBResponseBodyWriter an inout? This would make it harder to incorrectly wield this API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you are getting at there. What do we gain from making any HBResponseBodyWriter inout. That closure is called by HTTPChannelHandler and any Middleware that want to process response bodies. ie very few people will be calling it.

Are you seeing a situation where you would want to the write function to mutate the HBResponseBodyWriter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't see a situation where we'd want to mutate. I see a risk in people trying to escape that HBResponseBodyWriter out of this closure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to hold up this PR for this topic though, since it's not caused by this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I see, by making it inout you make it harder but it could still be done. I could make ~Copyable I don't know if that'd help

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot that HBResponseBodyWriter is a protocol not a struct

Copy link
Member Author

@adam-fowler adam-fowler Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an issue to chase this up #342

) -> Self {
self.init(contentLength: contentLength, write: write)
}

/// Initialise HBResponseBody with closure writing body contents
///
/// This version of init is private and only available via ``withTrailingHeaders`` because
/// if it is public the compiler gets confused when a complex closure is provided.
private init(contentLength: Int? = nil, write: @Sendable @escaping (any HBResponseBodyWriter) async throws -> HTTPFields?) {
self.write = { return try await write($0) }
self.contentLength = contentLength
}
}
4 changes: 2 additions & 2 deletions Sources/HummingbirdCore/Server/HTTP/HTTPChannelHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ extension HTTPChannelHandler {
}
do {
try await outbound.write(.head(response.head))
try await response.body.write(responseWriter)
try await outbound.write(.end(nil))
let tailHeaders = try await response.body.write(responseWriter)
try await outbound.write(.end(tailHeaders))
// flush request body
for try await _ in request.body {}
} catch {
Expand Down
4 changes: 2 additions & 2 deletions Sources/HummingbirdXCT/Application+XCT.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ public struct XCTTestingSetup {

/// Test writing requests directly to router.
public static var router: XCTTestingSetup { .init(value: .router) }
/// Sets up a live server and execute tests using a HTTP client.
/// Sets up a live server and execute tests using a HTTP client. Only supports HTTP1
public static var live: XCTTestingSetup { .init(value: .live) }
/// Sets up a live server and execute tests using a HTTP client.
/// Sets up a live server and execute tests using a HTTP client. Does not support trailer headers
public static func ahc(_ scheme: XCTScheme) -> XCTTestingSetup { .init(value: .ahc(scheme)) }
}

Expand Down
2 changes: 2 additions & 0 deletions Sources/HummingbirdXCT/HBXCTApplication.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public struct HBXCTResponse: Sendable {
public var headers: HTTPFields { self.head.headerFields }
/// response body
public let body: ByteBuffer?
/// trailer headers
public let trailerHeaders: HTTPFields?
}

/// Errors thrown by XCT framework.
Expand Down
2 changes: 1 addition & 1 deletion Sources/HummingbirdXCT/HBXCTAsyncHTTPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ final class HBXCTAsyncHTTPClient<App: HBApplicationProtocol>: HBXCTApplication {
request.body = body.map { .bytes($0) }
let response = try await client.execute(request, deadline: .now() + self.timeout)
let responseHead = HTTPResponseHead(version: response.version, status: response.status, headers: response.headers)
return try await .init(head: .init(responseHead), body: response.body.collect(upTo: .max))
return try await .init(head: .init(responseHead), body: response.body.collect(upTo: .max), trailerHeaders: nil)
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/HummingbirdXCT/HBXCTLive.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ final class HBXCTLive<App: HBApplicationProtocol>: HBXCTApplication {
headers[.connection] = "keep-alive"
let request = HBXCTClient.Request(uri, method: method, authority: "localhost", headers: headers, body: body)
let response = try await client.execute(request)
return .init(head: response.head, body: response.body)
return .init(head: response.head, body: response.body, trailerHeaders: response.trailerHeaders)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/HummingbirdXCT/HBXCTRouter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,10 @@ struct HBXCTRouter<Responder: HBResponder>: HBXCTApplication where Responder.Con
response = HBResponse(status: .internalServerError)
}
let responseWriter = RouterResponseWriter()
try await response.body.write(responseWriter)
let trailerHeaders = try await response.body.write(responseWriter)
for try await _ in request.body {}
return responseWriter.collated.withLockedValue { collated in
HBXCTResponse(head: response.head, body: collated)
HBXCTResponse(head: response.head, body: collated, trailerHeaders: trailerHeaders)
}
}

Expand Down
4 changes: 3 additions & 1 deletion Sources/HummingbirdXCT/XCTClient+types.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@ extension HBXCTClient {
public struct Response: Sendable {
public var head: HTTPResponse
public var body: ByteBuffer?
public var trailerHeaders: HTTPFields?

public init(head: HTTPResponse, body: ByteBuffer? = nil) {
public init(head: HTTPResponse, body: ByteBuffer? = nil, trailerHeaders: HTTPFields? = nil) {
self.head = head
self.body = body
self.trailerHeaders = trailerHeaders
}

public var status: HTTPResponse.Status {
Expand Down
12 changes: 6 additions & 6 deletions Sources/HummingbirdXCT/XCTClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -234,21 +234,21 @@ public struct HBXCTClient: Sendable {
case (.body(var part), .body(let head, var body)):
body.writeBuffer(&part)
self.state = .body(head, body)
case (.end(let tailHeaders), .body(let head, let body)):
assert(tailHeaders == nil, "Unexpected tail headers")
case (.end(let trailerHeaders), .body(let head, let body)):
let response = HBXCTClient.Response(
head: head,
body: body
body: body,
trailerHeaders: trailerHeaders
)
if context.channel.isActive {
context.fireChannelRead(wrapInboundOut(response))
}
self.state = .idle
case (.end(let tailHeaders), .head(let head)):
assert(tailHeaders == nil, "Unexpected tail headers")
case (.end(let trailerHeaders), .head(let head)):
let response = HBXCTClient.Response(
head: head,
body: nil
body: nil,
trailerHeaders: trailerHeaders
)
if context.channel.isActive {
context.fireChannelRead(wrapInboundOut(response))
Expand Down
13 changes: 13 additions & 0 deletions Tests/HummingbirdCoreTests/CoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,19 @@ class HummingBirdCoreTests: XCTestCase {
}
}

func testTrailerHeaders() async throws {
try await testServer(
responder: { _, _ in .init(status: .ok, body: .withTrailingHeaders { _ in return [.contentType: "text"] }) },
httpChannelSetup: .http1(),
configuration: .init(address: .hostname(port: 0)),
eventLoopGroup: Self.eventLoopGroup,
logger: Logger(label: "HB")
) { client in
let response = try await client.get("/")
XCTAssertEqual(response.trailerHeaders?[.contentType], "text")
}
}

func testChannelHandlerErrorPropagation() async throws {
class CreateErrorHandler: ChannelInboundHandler, RemovableChannelHandler {
typealias InboundIn = HTTPRequestPart
Expand Down
5 changes: 3 additions & 2 deletions Tests/HummingbirdRouterTests/MiddlewareTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,10 @@ final class MiddlewareTests: XCTestCase {
func handle(_ request: HBRequest, context: Context, next: (HBRequest, Context) async throws -> HBResponse) async throws -> HBResponse {
let response = try await next(request, context)
var editedResponse = response
editedResponse.body = .init { writer in
editedResponse.body = .withTrailingHeaders { writer in
let transformWriter = TransformWriter(parentWriter: writer, allocator: context.allocator)
try await response.body.write(transformWriter)
let tailHeaders = try await response.body.write(transformWriter)
return tailHeaders
}
return editedResponse
}
Expand Down
5 changes: 3 additions & 2 deletions Tests/HummingbirdTests/MiddlewareTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,10 @@ final class MiddlewareTests: XCTestCase {
public func handle(_ request: HBRequest, context: Context, next: (HBRequest, Context) async throws -> HBResponse) async throws -> HBResponse {
let response = try await next(request, context)
var editedResponse = response
editedResponse.body = .init { writer in
editedResponse.body = .withTrailingHeaders { writer in
let transformWriter = TransformWriter(parentWriter: writer, allocator: context.allocator)
try await response.body.write(transformWriter)
let tailHeaders = try await response.body.write(transformWriter)
return tailHeaders
}
return editedResponse
}
Expand Down
Loading