diff --git a/.licenseignore b/.licenseignore index 09fef5ae..cdd11410 100644 --- a/.licenseignore +++ b/.licenseignore @@ -45,3 +45,4 @@ dev/update-benchmark-thresholds FuzzTesting/FailCases/* Tests/hpack-test-case/* .flake8 +dev/alloc-limits-from-test-output diff --git a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.0.json b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.0.json index ec24d5e1..f5410be1 100644 --- a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.0.json +++ b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.0.json @@ -3,8 +3,8 @@ "1k_requests_inline_noninterleaved": 29100, "1k_requests_interleaved": 36150, "1k_requests_noninterleaved": 35100, - "client_server_h1_request_response": 284050, - "client_server_h1_request_response_inline": 269050, + "client_server_h1_request_response": 275050, + "client_server_h1_request_response_inline": 263050, "client_server_request_response": 253050, "client_server_request_response_inline": 244050, "client_server_request_response_many": 1198050, @@ -18,6 +18,6 @@ "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050, "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050, "hpack_decoding": 5050, - "stream_teardown_100_concurrent": 253550, + "stream_teardown_100_concurrent": 253250, "stream_teardown_100_concurrent_inline": 252350 } diff --git a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.1.json b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.1.json index ec24d5e1..f5410be1 100644 --- a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.1.json +++ b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.1.json @@ -3,8 +3,8 @@ "1k_requests_inline_noninterleaved": 29100, "1k_requests_interleaved": 36150, "1k_requests_noninterleaved": 35100, - "client_server_h1_request_response": 284050, - "client_server_h1_request_response_inline": 269050, + "client_server_h1_request_response": 275050, + "client_server_h1_request_response_inline": 263050, "client_server_request_response": 253050, "client_server_request_response_inline": 244050, "client_server_request_response_many": 1198050, @@ -18,6 +18,6 @@ "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050, "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050, "hpack_decoding": 5050, - "stream_teardown_100_concurrent": 253550, + "stream_teardown_100_concurrent": 253250, "stream_teardown_100_concurrent_inline": 252350 } diff --git a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.2.json b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.2.json index ec24d5e1..f5410be1 100644 --- a/IntegrationTests/tests_01_allocation_counters/Thresholds/6.2.json +++ b/IntegrationTests/tests_01_allocation_counters/Thresholds/6.2.json @@ -3,8 +3,8 @@ "1k_requests_inline_noninterleaved": 29100, "1k_requests_interleaved": 36150, "1k_requests_noninterleaved": 35100, - "client_server_h1_request_response": 284050, - "client_server_h1_request_response_inline": 269050, + "client_server_h1_request_response": 275050, + "client_server_h1_request_response_inline": 263050, "client_server_request_response": 253050, "client_server_request_response_inline": 244050, "client_server_request_response_many": 1198050, @@ -18,6 +18,6 @@ "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050, "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050, "hpack_decoding": 5050, - "stream_teardown_100_concurrent": 253550, + "stream_teardown_100_concurrent": 253250, "stream_teardown_100_concurrent_inline": 252350 } diff --git a/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-main.json b/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-main.json index ec24d5e1..f5410be1 100644 --- a/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-main.json +++ b/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-main.json @@ -3,8 +3,8 @@ "1k_requests_inline_noninterleaved": 29100, "1k_requests_interleaved": 36150, "1k_requests_noninterleaved": 35100, - "client_server_h1_request_response": 284050, - "client_server_h1_request_response_inline": 269050, + "client_server_h1_request_response": 275050, + "client_server_h1_request_response_inline": 263050, "client_server_request_response": 253050, "client_server_request_response_inline": 244050, "client_server_request_response_many": 1198050, @@ -18,6 +18,6 @@ "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050, "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050, "hpack_decoding": 5050, - "stream_teardown_100_concurrent": 253550, + "stream_teardown_100_concurrent": 253250, "stream_teardown_100_concurrent_inline": 252350 } diff --git a/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-next.json b/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-next.json index ec24d5e1..f5410be1 100644 --- a/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-next.json +++ b/IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-next.json @@ -3,8 +3,8 @@ "1k_requests_inline_noninterleaved": 29100, "1k_requests_interleaved": 36150, "1k_requests_noninterleaved": 35100, - "client_server_h1_request_response": 284050, - "client_server_h1_request_response_inline": 269050, + "client_server_h1_request_response": 275050, + "client_server_h1_request_response_inline": 263050, "client_server_request_response": 253050, "client_server_request_response_inline": 244050, "client_server_request_response_many": 1198050, @@ -18,6 +18,6 @@ "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050, "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050, "hpack_decoding": 5050, - "stream_teardown_100_concurrent": 253550, + "stream_teardown_100_concurrent": 253250, "stream_teardown_100_concurrent_inline": 252350 } diff --git a/Sources/NIOHTTP2/HTTP2ToHTTP1Codec.swift b/Sources/NIOHTTP2/HTTP2ToHTTP1Codec.swift index ea8c95a5..1985be14 100644 --- a/Sources/NIOHTTP2/HTTP2ToHTTP1Codec.swift +++ b/Sources/NIOHTTP2/HTTP2ToHTTP1Codec.swift @@ -2,7 +2,7 @@ // // This source file is part of the SwiftNIO open source project // -// Copyright (c) 2017-2021 Apple Inc. and the SwiftNIO project authors +// Copyright (c) 2017-2026 Apple Inc. and the SwiftNIO project authors // Licensed under Apache License v2.0 // // See LICENSE.txt for license information @@ -23,9 +23,23 @@ private struct BaseClientCodec { private let normalizeHTTPHeaders: Bool private var headerStateMachine: HTTP2HeadersStateMachine = HTTP2HeadersStateMachine(mode: .client) - private var outgoingHTTP1RequestHead: HTTPRequestHead? + struct PendingFrameWrite { + var frame: HTTP2Frame.FramePayload + var promise: EventLoopPromise? + + init(_ frame: HTTP2Frame.FramePayload, _ promise: EventLoopPromise?) { + self.frame = frame + self.promise = promise + } + } + + /// Caches the most recent outbound frame until flush or .end is received. + /// This allows us to set endStream=true on the final data/headers frame + /// instead of sending a separate empty data frame just to signal stream closure. + private var pendingFrameWrite: PendingFrameWrite? + /// Initializes a `BaseClientCodec`. /// /// - Parameters: @@ -102,8 +116,8 @@ private struct BaseClientCodec { mutating func processOutboundData( _ data: HTTPClientRequestPart, - allocator: ByteBufferAllocator - ) throws -> HTTP2Frame.FramePayload { + promise: EventLoopPromise? + ) throws -> (first: PendingFrameWrite?, second: PendingFrameWrite?) { switch data { case .head(let head): precondition(self.outgoingHTTP1RequestHead == nil, "Only a single HTTP request allowed per HTTP2 stream") @@ -115,25 +129,73 @@ private struct BaseClientCodec { normalizeHTTPHeaders: self.normalizeHTTPHeaders ) ) - return .headers(headerContent) + self.pendingFrameWrite = .init(.headers(headerContent), promise) + return (nil, nil) + case .body(let body): - return .data(HTTP2Frame.FramePayload.Data(data: body)) + let cached = self.pendingFrameWrite + self.pendingFrameWrite = .init(.data(HTTP2Frame.FramePayload.Data(data: body)), promise) + return (cached, nil) + case .end(let trailers): - if let trailers = trailers { - return .headers( - .init( - headers: HPACKHeaders( - httpHeaders: trailers, - normalizeHTTPHeaders: self.normalizeHTTPHeaders - ), - endStream: true - ) + defer { self.pendingFrameWrite = nil } + + switch (self.pendingFrameWrite?.frame, trailers) { + case (.none, .none): + return (.init(.data(.init(data: .byteBuffer(ByteBuffer()), endStream: true)), promise), nil) + + case (.none, .some(let trailers)): + return (.init(self.makeH2TrailerFramePayload(trailers), promise), nil) + + case (.data(var data), .none): + data.endStream = true + var pendingPromise = self.pendingFrameWrite!.promise + pendingPromise.setOrCascade(to: promise) + return (.init(.data(data), pendingPromise), nil) + + case (.data(let data), .some(let trailers)): + let trailerFrame = self.makeH2TrailerFramePayload(trailers) + return ( + .init(.data(data), self.pendingFrameWrite!.promise), + .init(trailerFrame, promise) ) - } else { - return .data(.init(data: .byteBuffer(allocator.buffer(capacity: 0)), endStream: true)) + + case (.headers(var headers), .none): + headers.endStream = true + var pendingPromise = self.pendingFrameWrite!.promise + pendingPromise.setOrCascade(to: promise) + return (.init(.headers(headers), pendingPromise), nil) + + case (.headers(let headers), .some(let trailers)): + let trailers = self.makeH2TrailerFramePayload(trailers) + return ( + .init(.headers(headers), self.pendingFrameWrite!.promise), + .init(trailers, promise) + ) + + case (.priority, _), (.rstStream, _), (.settings, _), (.pushPromise, _), (.ping, _), (.goAway, _), + (.windowUpdate, _), (.alternativeService, _), (.origin, _): + fatalError("Only header and data frames are cached here") } } } + + mutating func clearCache() -> PendingFrameWrite? { + defer { self.pendingFrameWrite = nil } + return self.pendingFrameWrite + } + + private func makeH2TrailerFramePayload(_ trailers: HTTPHeaders) -> HTTP2Frame.FramePayload { + HTTP2Frame.FramePayload.headers( + .init( + headers: HPACKHeaders( + httpHeaders: trailers, + normalizeHTTPHeaders: self.normalizeHTTPHeaders + ), + endStream: true + ) + ) + } } /// A simple channel handler that translates HTTP/2 concepts into HTTP/1 data types, @@ -199,17 +261,35 @@ public final class HTTP2ToHTTP1ClientCodec: ChannelInboundHandler, ChannelOutbou let responsePart = self.unwrapOutboundIn(data) do { - let transformedPayload = try self.baseCodec.processOutboundData( - responsePart, - allocator: context.channel.allocator - ) - let part = HTTP2Frame(streamID: self.streamID, payload: transformedPayload) - context.write(self.wrapOutboundOut(part), promise: promise) + let (first, second) = try self.baseCodec.processOutboundData(responsePart, promise: promise) + if let first = first { + let part = HTTP2Frame(streamID: self.streamID, payload: first.frame) + context.write(self.wrapOutboundOut(part), promise: first.promise) + if let second = second { + let part = HTTP2Frame(streamID: self.streamID, payload: second.frame) + context.write(self.wrapOutboundOut(part), promise: second.promise) + } + } } catch { promise?.fail(error) context.fireErrorCaught(error) } } + + public func flush(context: ChannelHandlerContext) { + if let pending = self.baseCodec.clearCache() { + let part = HTTP2Frame(streamID: self.streamID, payload: pending.frame) + context.write(self.wrapOutboundOut(part), promise: pending.promise) + } + context.flush() + } + + public func errorCaught(context: ChannelHandlerContext, error: any Error) { + if let pending = self.baseCodec.clearCache() { + pending.promise?.fail(error) + } + context.fireErrorCaught(error) + } } @available(*, unavailable) @@ -270,16 +350,35 @@ public final class HTTP2FramePayloadToHTTP1ClientCodec: ChannelInboundHandler, C let requestPart = self.unwrapOutboundIn(data) do { - let transformedPayload = try self.baseCodec.processOutboundData( + let (first, second) = try self.baseCodec.processOutboundData( requestPart, - allocator: context.channel.allocator + promise: promise ) - context.write(self.wrapOutboundOut(transformedPayload), promise: promise) + if let first { + context.write(self.wrapOutboundOut(first.frame), promise: first.promise) + if let second { + context.write(self.wrapOutboundOut(second.frame), promise: second.promise) + } + } } catch { promise?.fail(error) context.fireErrorCaught(error) } } + + public func flush(context: ChannelHandlerContext) { + if let pending = self.baseCodec.clearCache() { + context.write(self.wrapOutboundOut(pending.frame), promise: pending.promise) + } + context.flush() + } + + public func errorCaught(context: ChannelHandlerContext, error: any Error) { + if let pending = self.baseCodec.clearCache() { + pending.promise?.fail(error) + } + context.fireErrorCaught(error) + } } @available(*, unavailable) diff --git a/Tests/NIOHTTP2Tests/HTTP2FramePayloadToHTTP1CodecTests.swift b/Tests/NIOHTTP2Tests/HTTP2FramePayloadToHTTP1CodecTests.swift index 994deac5..de069256 100644 --- a/Tests/NIOHTTP2Tests/HTTP2FramePayloadToHTTP1CodecTests.swift +++ b/Tests/NIOHTTP2Tests/HTTP2FramePayloadToHTTP1CodecTests.swift @@ -2,7 +2,7 @@ // // This source file is part of the SwiftNIO open source project // -// Copyright (c) 2020-2021 Apple Inc. and the SwiftNIO project authors +// Copyright (c) 2020-2026 Apple Inc. and the SwiftNIO project authors // Licensed under Apache License v2.0 // // See LICENSE.txt for license information @@ -1364,4 +1364,138 @@ final class HTTP2FramePayloadToHTTP1CodecTests: XCTestCase { } } + func testHeadersEndStreamIfFollowedByEndImmediately() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let headPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: headPromise) + channel.write(HTTPClientRequestPart.end(nil), promise: nil) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: true, headers: expectedRequestHeaders) + XCTAssertEqual(writeRecorder.flushedWrites.count, 1) + XCTAssertNoThrow(try headPromise.futureResult.wait()) + } + + func testHeadersDontEndStreamIfFlushedEvenWhenFollowedByEnd() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let headPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: headPromise) + channel.flush() + XCTAssertNoThrow(try headPromise.futureResult.wait()) + channel.write(HTTPClientRequestPart.end(nil), promise: nil) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: false, headers: expectedRequestHeaders) + writeRecorder.flushedWrites[1].assertDataFramePayload(endStream: true, payload: ByteBuffer()) + XCTAssertEqual(writeRecorder.flushedWrites.count, 2) + } + + func testHeadersDontEndStreamIfFollowedByEndWithTrailersImmediately() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let headPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: headPromise) + channel.write(HTTPClientRequestPart.end(["foo": "bar"]), promise: nil) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: false, headers: expectedRequestHeaders) + writeRecorder.flushedWrites[1].assertHeadersFramePayload(endStream: true, headers: ["foo": "bar"]) + XCTAssertEqual(writeRecorder.flushedWrites.count, 2) + XCTAssertNoThrow(try headPromise.futureResult.wait()) + } + + func testDataFrameEndsStreamIfFollowedByEndImmediately() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let body = ByteBuffer(string: "Hello World!") + let bodyPromise = channel.eventLoop.makePromise(of: Void.self) + let endPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: nil) + channel.write(HTTPClientRequestPart.body(.byteBuffer(body)), promise: bodyPromise) + channel.write(HTTPClientRequestPart.end(nil), promise: endPromise) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: false, headers: expectedRequestHeaders) + writeRecorder.flushedWrites[1].assertDataFramePayload(endStream: true, payload: body) + XCTAssertEqual(writeRecorder.flushedWrites.count, 2) + XCTAssertNoThrow(try bodyPromise.futureResult.wait()) + XCTAssertNoThrow(try endPromise.futureResult.wait()) + } + + func testDataFrameDoesntEndStreamIfFlushedEvenWhenFollowedByEnd() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let body = ByteBuffer(string: "Hello World!") + let bodyPromise = channel.eventLoop.makePromise(of: Void.self) + let endPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: nil) + channel.write(HTTPClientRequestPart.body(.byteBuffer(body)), promise: bodyPromise) + channel.flush() + channel.write(HTTPClientRequestPart.end(nil), promise: endPromise) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: false, headers: expectedRequestHeaders) + writeRecorder.flushedWrites[1].assertDataFramePayload(endStream: false, payload: body) + writeRecorder.flushedWrites[2].assertDataFramePayload(endStream: true, payload: ByteBuffer()) + XCTAssertEqual(writeRecorder.flushedWrites.count, 3) + XCTAssertNoThrow(try bodyPromise.futureResult.wait()) + XCTAssertNoThrow(try endPromise.futureResult.wait()) + } + + func testDataFrameDoesntEndStreamIfFollowedByEndWithTrailersImmediately() throws { + let handler = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https) + let writeRecorder = FramePayloadWriteRecorder() + let channel = EmbeddedChannel(handlers: [writeRecorder, handler]) + + let http1Head = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/", headers: ["host": "example.org"]) + let body = ByteBuffer(string: "Hello World!") + let bodyPromise = channel.eventLoop.makePromise(of: Void.self) + let endPromise = channel.eventLoop.makePromise(of: Void.self) + let expectedRequestHeaders = HPACKHeaders([ + (":method", "GET"), (":path", "/"), (":authority", "example.org"), (":scheme", "https"), + ]) + + channel.write(HTTPClientRequestPart.head(http1Head), promise: nil) + channel.write(HTTPClientRequestPart.body(.byteBuffer(body)), promise: bodyPromise) + channel.write(HTTPClientRequestPart.end(["foo": "bar"]), promise: endPromise) + channel.flush() + writeRecorder.flushedWrites[0].assertHeadersFramePayload(endStream: false, headers: expectedRequestHeaders) + writeRecorder.flushedWrites[1].assertDataFramePayload(endStream: false, payload: body) + writeRecorder.flushedWrites[2].assertHeadersFramePayload(endStream: true, headers: ["foo": "bar"]) + XCTAssertEqual(writeRecorder.flushedWrites.count, 3) + XCTAssertNoThrow(try bodyPromise.futureResult.wait()) + XCTAssertNoThrow(try endPromise.futureResult.wait()) + } }