HTTP/2 negotiated connection pipeline config #408

rnro · 2023-07-05T12:57:58Z

Motivation:

This continues the work to expose functionality which allows users to interact with HTTP/2 connections via async abstractions and using structured concurrency.

This work enables protocol negotiation between HTTP/1.1 and HTTP/2 in its most generic form.

Modifications:

Enable protocol negotiation between HTTP/1.1 and HTTP/2 using typed negotiation results (NIOProtocolNegotiationResult) from the NIOTypedApplicationProtocolNegotiationHandler.
In the HTTP/2 case the negotiation result will return the HTTP/2 connection channel and the NIOHTTP2Handler.AsyncStreamMultiplexer which exposes inbound streams as an iterable async stream.

Result:

Users will be able to set up negotiated HTTP/1.1 / HTTP/2 connections and iterate over inbound streams.

FranzBusch · 2023-07-05T13:12:05Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -380,6 +380,63 @@ extension Channel {
        return self.pipeline.addHandler(alpnHandler)
    }

+
+    /// Configures a channel to perform a HTTP/2 secure upgrade with typed negotiation results.


NIT

Suggested change

/// Configures a channel to perform a HTTP/2 secure upgrade with typed negotiation results.

/// Configures a channel to perform an HTTP/2 secure upgrade with typed negotiation results.

FranzBusch · 2023-07-05T13:14:05Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+    ///     the negotiation
+    @_spi(AsyncChannel)
+    public func configureHTTP2AsyncSecureUpgrade<HTTP1Output, HTTP2Output>(
+        h2ChannelConfigurator: @escaping (Channel) throws -> EventLoopFuture<HTTP2Output>,


Can we call this http2ChannelConfigurator to align with the other parameter. I think these closures also need to be @Sendable.

FranzBusch · 2023-07-05T13:21:18Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+/// `NegotiationResult` is a generic negotiation result holder for HTTP/1.1 and HTTP/2
+@_spi(AsyncChannel)
+public enum NegotiationResult<HTTP1Output, HTTP2Output> {
+    case http1_1(HTTP1Output)
+    case http2(HTTP2Output)
+}


I think we need to be a bit more specific here. Especially if we want to have something that does H1, H2, H3 in the future. What about naming this HTTP1And2NegotiationResult? Also open for better names cc @Lukasa @glbrntt

We need a NIO prefix at least. I'm not sure if we need to be quite so wary of H3 since it will be a new package/module and so won't need a NIO prefix.

How about NIONegotiatedHTTPVersion? Or NIOHTTPNegotiationResult? I prefer the former because it doesn't start with 8 capital letters...

I've gone with NIONegotiatedHTTPVersion

FranzBusch · 2023-07-05T13:34:28Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

-    ) -> EventLoopFuture<Void> {
-        return self._commonHTTPServerPipeline(configurator: configurator, h2ConnectionChannelConfigurator: h2ConnectionChannelConfigurator) { channel in
-            channel.configureHTTP2Pipeline(
+        h1ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP1Output>,


This uses a type on the H2 handler which is a bit confusing.

I've created a pair of simplified global types which are used everywhere except for where the existing NIOHTTP2Handler.StreamInitializer was already used.

FranzBusch · 2023-07-05T13:34:45Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

    ///   - h2ConnectionChannelConfigurator: An optional callback that will be invoked only
-    ///         when the negotiated protocol is H2 to configure the connection channel.
+    ///         when the negotiated protocol is HTTP/2 to configure the connection channel.
    ///   - inboundStreamInitializer: A closure that will be called whenever the remote peer initiates a new stream.


This doc comment doesn't match with the parameter names

I think they do but that the diff is presenting them in a confusing way

I restructured the original commit to split out the simple moving of a function to group like functions. Hopefully that makes the main diff easier to understand

FranzBusch · 2023-07-05T13:35:27Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

-            channel.configureHTTP2Pipeline(
+        h1ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP1Output>,
+        h2ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP2Output>,
+        streamInitializer: @escaping NIOHTTP2Handler.StreamInitializerWithOutput<HTTP2StreamOutput>


I think we should holistically prefix the parameters with either h1/2 or http1/http2 if they are for one specific protocol. I got quite confused trying to understand what parameter applies to what

Yup, agree, streamInitializer is not obviously http2

FranzBusch · 2023-07-05T13:36:37Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+        h1ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP1Output>,
+        h2ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP2Output>,
+        streamInitializer: @escaping NIOHTTP2Handler.StreamInitializerWithOutput<HTTP2StreamOutput>
+    ) throws -> EventLoopFuture<NIOProtocolNegotiationResult<NegotiationResult<HTTP1Output, (HTTP2Output, NIOHTTP2Handler.AsyncStreamMultiplexer<HTTP2StreamOutput>)>>> {


I am a bit surprised by this return type. I rather expected us to return the handler instead of the result here. The reason for this is that our bootstrap methods require the channel initialiser to return a Handler which we then use to await for the result.

I've changed this to match the bootstrap methods.

glbrntt · 2023-07-05T16:51:05Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -434,6 +491,47 @@ extension Channel {
        }
    }

+    /// Configures a `ChannelPipeline` to speak either HTTP or HTTP/2 according to what can be negotiated with the client.


Suggested change

/// Configures a `ChannelPipeline` to speak either HTTP or HTTP/2 according to what can be negotiated with the client.

/// Configures a `ChannelPipeline` to speak either HTTP/1.1 or HTTP/2 according to what can be negotiated with the client.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

glbrntt · 2023-07-05T16:56:58Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

-            channel.configureHTTP2Pipeline(
+        h1ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP1Output>,
+        h2ConnectionChannelConfigurator: @escaping NIOHTTP2Handler.ConnectionInitializerWithOutput<HTTP2Output>,
+        streamInitializer: @escaping NIOHTTP2Handler.StreamInitializerWithOutput<HTTP2StreamOutput>


Yup, agree, streamInitializer is not obviously http2

FranzBusch · 2023-07-06T10:48:33Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+    ///     is ready to negotiate. This can then be used to access the ``NIOProtocolNegotiationResult`` which may itself
+    ///     be waited on to retrieve the result of the negotiation
+    @_spi(AsyncChannel)
+    public func configureHTTP2AsyncSecureUpgrade<HTTP1Output, HTTP2Output>(


Are we intending to make this public in the end? I don't see a lot of usage for this without adding the protocol handlers in there so maybe we should make it non public in the end and right now add an _ to the method name?

I've made this internal for now. It was originally public in parity with the non-async function but we can see if there's a need to make it public.

FranzBusch · 2023-07-06T10:49:18Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

        connectionConfiguration: NIOHTTP2Handler.ConnectionConfiguration,
        streamConfiguration: NIOHTTP2Handler.StreamConfiguration,
        streamDelegate: NIOHTTP2StreamDelegate? = nil,


IMO we should prefix those methods with http2 as well. They only apply to it and right now you can only discover this by reading the docs.

Configuration has been largely refactored.

FranzBusch · 2023-07-06T10:50:21Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+    ///     be waited on to retrieve the result of the negotiation
+    @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+    @_spi(AsyncChannel)
+    public func configureAsyncHTTPServerPipeline<HTTP1Output, HTTP2Output, HTTP2StreamOutput>(


What about naming the HTTP2Ouput generic type as HTTP2ConnectionOutput to better differentiate the two H2 types. (Maybe also add it for the H1 type

FranzBusch · 2023-07-06T10:52:31Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

-            channel.configureHTTP2Pipeline(
+        http1ChannelConfigurator: @escaping NIOChannelInitializerWithOutput<HTTP1Output>,
+        http2ChannelConfigurator: @escaping NIOChannelInitializerWithOutput<HTTP2Output>,
+        http2StreamInitializer: @escaping NIOChannelInitializerWithOutput<HTTP2StreamOutput>


Should we add inbound in this parameters name?

Configuration has been largely refactored.

FranzBusch · 2023-07-06T10:54:54Z

Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift

+    class HTTP1ServerRequestRecorderHandler: ChannelInboundHandler {
+        typealias InboundIn = HTTPServerRequestPart
+
+        var receivedParts: [HTTPServerRequestPart] = []
+
+        func channelRead(context: ChannelHandlerContext, data: NIOAny) {
+            self.receivedParts.append(self.unwrapInboundIn(data))
+            context.fireChannelRead(data)
+        }
+    }
+
+    /// A simple channel handler that records inbound frames.
+    class HTTP1ClientResponseRecorderHandler: ChannelInboundHandler {


Suggested change

class HTTP1ServerRequestRecorderHandler: ChannelInboundHandler {

typealias InboundIn = HTTPServerRequestPart

var receivedParts: [HTTPServerRequestPart] = []

func channelRead(context: ChannelHandlerContext, data: NIOAny) {

self.receivedParts.append(self.unwrapInboundIn(data))

context.fireChannelRead(data)

}

}

/// A simple channel handler that records inbound frames.

class HTTP1ClientResponseRecorderHandler: ChannelInboundHandler {

final class HTTP1ServerRequestRecorderHandler: ChannelInboundHandler {

typealias InboundIn = HTTPServerRequestPart

var receivedParts: [HTTPServerRequestPart] = []

func channelRead(context: ChannelHandlerContext, data: NIOAny) {

self.receivedParts.append(self.unwrapInboundIn(data))

context.fireChannelRead(data)

}

}

/// A simple channel handler that records inbound frames.

final class HTTP1ClientResponseRecorderHandler: ChannelInboundHandler {

FranzBusch · 2023-07-06T10:55:22Z

Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift

+        let clientRecorder = try await self.clientChannel.pipeline.handler(type: HTTP1ClientResponseRecorderHandler.self).get()
+        let serverRecorder = try await self.serverChannel.pipeline.handler(type: HTTP1ServerRequestRecorderHandler.self).get()


This is not safe. Those two types are not Sendable

I've made these recorder types Sendable

glbrntt · 2023-07-10T16:11:15Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

+        public var connection: ConnectionConfiguration = .init()
+        /// The settings that will be used when establishing new streams. These mainly pertain to flow control.
+        public var stream: StreamConfiguration = .init()
+        public init() {}


Given we're just group two configs here I think we'll be okay having them in the init too.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

glbrntt · 2023-07-11T09:46:33Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+    ///     be waited on to retrieve the result of the negotiation
+    @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+    @_spi(AsyncChannel)
+    public func configureAsyncHTTPServerPipeline<HTTP1ConnectionOutput, HTTP2ConnectionOutput, HTTP2StreamOutput>(


I think the existing version of this has a notion of "upgrade" in its name

The method which mentions "upgrade" is one layer down, the analogue of this is configureCommonHTTPServerPipeline - but we don't have any common configuration because we don't translate H2 into H1 so that's not relevant.

glbrntt · 2023-07-11T09:47:24Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -673,55 +725,112 @@ extension ChannelPipeline.SynchronousOperations {
    /// 
    /// Using this rather than implementing a similar function yourself allows that pipeline to evolve without breaking your code.
    ///
+


Suggested change

glbrntt · 2023-07-11T09:49:02Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+/// `HTTP2AsyncConfiguration` contains all configuration required for setting up an HTTP/2 async connection.
+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+@_spi(AsyncChannel)
+public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound, HTTP2ConnectionOutbound, HTTP2StreamInbound, HTTP2StreamOutbound> {


Suggested change

public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound, HTTP2ConnectionOutbound, HTTP2StreamInbound, HTTP2StreamOutbound> {

public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound, HTTP2ConnectionOutbound, HTTP2StreamInbound, HTTP2StreamOutbound>: Sendable {

glbrntt · 2023-07-11T09:50:01Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+            inboundType: Inbound.Type = Inbound.self,
+            outboundType: Outbound.Type = Outbound.self,
+            backpressureStrategy: NIOAsyncSequenceProducerBackPressureStrategies.HighLowWatermark? = nil,
+            isOutboundHalfClosureEnabled: Bool? = nil


I think these were added in a different order in NIO: inboundType and outboundType came at the end

glbrntt · 2023-07-11T09:50:49Z

Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift

+        typealias OutboundOut = HTTPServerResponsePart
+
+        func channelRead(context: ChannelHandlerContext, data: NIOAny) {
+            guard case .end = self.unwrapInboundIn(data) else {


nit: prefer switching over all cases

glbrntt · 2023-07-11T09:53:36Z

Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift

+        typealias InboundIn = message
+
+        private let partsLock = NIOLock()
+        var _receivedParts: [message] = []


Suggested change

var _receivedParts: [message] = []

private var _receivedParts: [message] = []

glbrntt · 2023-07-11T09:54:54Z

Tests/NIOHTTP2Tests/TestUtilities.swift

@@ -64,6 +64,8 @@ extension XCTestCase {

    /// Have two `NIOAsyncTestingChannel` objects send and receive data from each other until
    /// they make no forward progress.
+    ///
+    /// ** This function is not thread safe and can lead to deadlocks, prefer the one-way variant which is less error-prone**


It is thread-safe, the problem is that calling it from multiple threads is racy.

glbrntt

over to you @FranzBusch

glbrntt · 2023-07-11T13:10:26Z

oh, looks like you regressed allocations @rnro

FranzBusch · 2023-07-11T13:32:18Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+/// `HTTP2AsyncConfiguration` contains all configuration required for setting up an HTTP/2 async connection.
+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+@_spi(AsyncChannel)
+public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound: Sendable, HTTP2ConnectionOutbound: Sendable, HTTP2StreamInbound: Sendable, HTTP2StreamOutbound: Sendable>: Sendable {


Suggested change

public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound: Sendable, HTTP2ConnectionOutbound: Sendable, HTTP2StreamInbound: Sendable, HTTP2StreamOutbound: Sendable>: Sendable {

public struct NIOHTTP2AsyncConfiguration<HTTP2ConnectionInbound: Sendable, HTTP2ConnectionOutbound: Sendable, HTTP2StreamInbound: Sendable, HTTP2StreamOutbound: Sendable>: Sendable {

rnro · 2023-07-11T14:42:45Z

@swift-server-bot test this please

Motivation: This continues the work to expose functionality which allows users to interact with HTTP/2 connections via async abstractions and using structured concurrency. This work enables protocol negotiation between HTTP/1.1 and HTTP/2 in its most generic form. Modifications: Enable protocol negotiation between HTTP/1.1 and HTTP/2 using typed negotiation results (`NIOProtocolNegotiationResult`) from the `NIOTypedApplicationProtocolNegotiationHandler`. In the HTTP/2 case the negotiation result will return the HTTP/2 connection channel and the `NIOHTTP2Handler.AsyncStreamMultiplexer` which exposes inbound streams as an iterable async stream. Result: Users will be able to set up negotiated HTTP/1.1 / HTTP/2 connections and iterate over inbound streams.

Large scale refactoring of code and function visibility to make the various APIs currently under SPI more manageable. As the API surface has become more complex with the addition of more layers it became apparent that we needed to take steps to simplify.

rnro · 2023-07-11T15:58:13Z

Rebased on top of #409

rnro · 2023-07-11T16:02:43Z

All of the API breakage is in code added since the last release. Almost all of it is Async... configuration functions, there are also some closure type aliases which they used but we've simplified and done away with.

glbrntt · 2023-07-12T07:00:11Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+#if swift(>=5.7)
+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+@_spi(AsyncChannel)
+extension NIOHTTP2AsyncConfiguration: @unchecked Sendable {}


nit: add a comment as to why this is @unchecked

rnro requested review from glbrntt and FranzBusch July 5, 2023 12:58

FranzBusch reviewed Jul 5, 2023

View reviewed changes

rnro force-pushed the async_channels_protocol_negotiation branch from 5ac7dd4 to d3d63d6 Compare July 5, 2023 14:44

glbrntt reviewed Jul 5, 2023

View reviewed changes

rnro requested review from glbrntt and FranzBusch July 6, 2023 08:12

rnro force-pushed the async_channels_protocol_negotiation branch from 36a06c1 to f6a2279 Compare July 6, 2023 09:40

FranzBusch reviewed Jul 6, 2023

View reviewed changes

rnro force-pushed the async_channels_protocol_negotiation branch from ee905c4 to b7da1fd Compare July 10, 2023 15:54

rnro requested a review from FranzBusch July 10, 2023 15:57

rnro force-pushed the async_channels_protocol_negotiation branch from b7da1fd to 8705984 Compare July 10, 2023 15:59

glbrntt reviewed Jul 11, 2023

View reviewed changes

rnro requested a review from glbrntt July 11, 2023 12:58

glbrntt approved these changes Jul 11, 2023

View reviewed changes

FranzBusch reviewed Jul 11, 2023

View reviewed changes

rnro requested a review from FranzBusch July 11, 2023 14:40

FranzBusch approved these changes Jul 11, 2023

View reviewed changes

rnro added 8 commits July 11, 2023 16:49

group configureCommonHTTPServerPipeline functions

d319896

review comments

b544df7

Refactoring and comments

a9efe3c

Large scale refactoring of code and function visibility to make the various APIs currently under SPI more manageable. As the API surface has become more complex with the addition of more layers it became apparent that we needed to take steps to simplify.

review comments

3509518

rename HTTP2AsyncConfiguration->NIOHTTP2AsyncConfiguration

39cab67

prbs

c6562c0

adopt NIOAsyncChannel.Configuration

97da616

rnro force-pushed the async_channels_protocol_negotiation branch from df3b529 to 97da616 Compare July 11, 2023 15:57

glbrntt approved these changes Jul 12, 2023

View reviewed changes

code comments

02e5d35

glbrntt merged commit 044339d into apple:main Jul 12, 2023
7 of 8 checks passed

rnro added the needs-no-version-bump For PRs that when merged do not need a bump in version number. label Oct 10, 2023

	/// Configures a channel to perform a HTTP/2 secure upgrade with typed negotiation results.
	/// Configures a channel to perform an HTTP/2 secure upgrade with typed negotiation results.

	/// Configures a `ChannelPipeline` to speak either HTTP or HTTP/2 according to what can be negotiated with the client.
	/// Configures a `ChannelPipeline` to speak either HTTP/1.1 or HTTP/2 according to what can be negotiated with the client.

		let clientRecorder = try await self.clientChannel.pipeline.handler(type: HTTP1ClientResponseRecorderHandler.self).get()
		let serverRecorder = try await self.serverChannel.pipeline.handler(type: HTTP1ServerRequestRecorderHandler.self).get()

	public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound, HTTP2ConnectionOutbound, HTTP2StreamInbound, HTTP2StreamOutbound> {
	public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound, HTTP2ConnectionOutbound, HTTP2StreamInbound, HTTP2StreamOutbound>: Sendable {

	var _receivedParts: [message] = []
	private var _receivedParts: [message] = []

	public struct HTTP2AsyncConfiguration<HTTP2ConnectionInbound: Sendable, HTTP2ConnectionOutbound: Sendable, HTTP2StreamInbound: Sendable, HTTP2StreamOutbound: Sendable>: Sendable {
	public struct NIOHTTP2AsyncConfiguration<HTTP2ConnectionInbound: Sendable, HTTP2ConnectionOutbound: Sendable, HTTP2StreamInbound: Sendable, HTTP2StreamOutbound: Sendable>: Sendable {

HTTP/2 negotiated connection pipeline config #408

HTTP/2 negotiated connection pipeline config #408

Conversation

rnro commented Jul 5, 2023

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

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

glbrntt left a comment

Choose a reason for hiding this comment

glbrntt commented Jul 11, 2023

Choose a reason for hiding this comment

rnro commented Jul 11, 2023

rnro commented Jul 11, 2023

rnro commented Jul 11, 2023

Choose a reason for hiding this comment