Async stream initializers order matches existing #415

rnro · 2023-08-11T07:01:30Z

Motivation:

When adding in the SPI methods for exposing async sequences of HTTP/2 streams we moved the stream initialization to a subtly different location so that it was easier to exfiltrate the outputs of those initialization functions (such as protocol negotiation outputs).

In some cases this broke ordering expectations.

Modifications:

Yield the (optionally wrapped) channels as initializer outputs to the async stream when the initialization and activation steps succeed.

Result:

Changes only exist within SPI. Async inbound stream channel initialization now matches previous behavior.

FranzBusch · 2023-08-11T07:05:34Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+            streamInitializerProductContinuation: continuation,
+            inboundStreamInitializer: { channel in
+                inboundStreamInitializer(channel).map { output in
+                    continuation.yield(output)


I don't think this ordering is correct here. The problem is that we might yield the new child channel to the continuation here but the activation might still fail for reasons inside the child channel.

What I have done in other places was to create a promise that we fulfill inside here and combine that with the promise of child channel creation. Once both promises are successful we can yield the output to the continuation.

Thanks for this feedback Franz. I went away and built something which I think fulfills these requirements. There's a slight wrinkle in the H/2 case which meant I had to start dealing with Any in a few places so that the generic Output type didn't leak into making the HTTP2Handler or StreamMultiplexer types generic.

rnro · 2023-08-17T16:31:43Z

The API breakage the checker is picking up is under SPI and I believe it is fine.

FranzBusch · 2023-08-23T12:55:27Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

@@ -243,7 +243,6 @@ extension NIOHTTP2Handler {
            return try await self.inlineStreamMultiplexer.createStreamChannel(initializer).get()
        }

-
        /// Create a stream channel initialized with the provided closure and return it wrapped within a `NIOAsyncChannel`.


I am surprised to still see this method. I thought we got rid of any NIOAsyncChannel APIs.

So am I but I didn't think tackling the issue of its continued existence was in-scope for this PR

Yeah sure, we should just make sure to remove this API here as well. I am fine if we do it in a follow up but maybe you could quickly check if this is just internal API right now and maybe RIP it out here as well?

After thinking about it, I just removed the API and fixed up the test

FranzBusch · 2023-08-23T13:45:39Z

Sources/NIOHTTP2/MultiplexerAbstractChannel.swift

+            let channelPromise: EventLoopPromise<Channel>?
+            if let promise = promise {
+                channelPromise = self.baseChannel.eventLoop.makePromise(of: Channel.self)
+                channelPromise!.completeWith(promise.futureResult.map { value in value as! Channel })
+            } else {
+                channelPromise = nil
+            }
+            self.baseChannel.configure(initializer: initializer, userPromise: channelPromise)
+        case .returnsAny(let initializer):
+            self.baseChannel.configure(initializer: initializer, userPromise: promise)


I am a bit confused here but might just lack the context. Are we using both cases because one is the inbound stream and the other is creating a new outbound stream case?

Sorry, I don't follow the question. Both cases of what? I'll take a punt at clarifying some things.

For initializer type:

.includesStreamID is the legacy case

.excludesStreamID is the non-async case

.returnsAny is the async case

We optionally remap the promise type if we have one from Any to Channel but only if we have one. In the past we didn't supply a promise on the inbound path so if we aren't in async world then we don't supply one now.

The thing I am confused then is why did something in the implementation for .excludesStreamID change now. If this is the non-async case can we actually hit this with a non-nil promise?

Ah, good point. No, we can't hit that in the non-async case, I'll remove it.

Motivation: When adding in the SPI methods for exposing async sequences of HTTP/2 streams we moved the stream initialization to a subtly different location so that it was easier to exfiltrate the outputs of those initialization functions (such as protocol negotiation outputs). In some cases this broke ordering expectations. Modifications: Yield the (optionally wrapped) channels as initializer outputs to the async stream Result: Changes only exist within SPI. Async inbound stream channel initialization now matches previous behavior.

This reverts commit 53be8d4.

Motivation: When adding in the SPI methods for exposing async sequences of HTTP/2 streams we moved the stream initialization to a subtly different location so that it was easier to exfiltrate the outputs of those initialization functions (such as protocol negotiation outputs). In some cases this broke ordering expectations. Modifications: Yield the (optionally wrapped) channels as initializer outputs to the async stream when the initialization and activation steps succeed. Result: Changes only exist within SPI. Async inbound stream channel initialization now matches previous behavior.

and other review comments

glbrntt · 2023-08-29T08:47:19Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -989,9 +1004,13 @@ extension NIOHTTP2Handler {
 #if swift(>=5.7)
    /// The type of all `inboundStreamInitializer` callbacks which do not need to return data.
    public typealias StreamInitializer = NIOChannelInitializer
+    /// The type of NIO Channel initializer callbacks which need to return untyped data.
+    internal typealias StreamInitializerWithAnyOutput = @Sendable (Channel) -> EventLoopFuture<Any>


Trying to think ahead with strict concurrency checking: should the return be EventLoopFuture<any Sendable> rather than just Any? Otherwise the closure cannot be Sendable.

Yeah that makes sense to me

glbrntt · 2023-08-29T08:54:32Z

Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift

            // Note: Firing the initial (header) frame before calling `HTTP2StreamChannel.configureInboundStream(initializer:)`
            // is crucial to preserve frame order, since the initialization process might trigger another read on the parent
            // channel which in turn might cause further frames to be processed synchronously.
            channel.receiveInboundFrame(frame)
-            channel.configureInboundStream(initializer: self.inboundStreamStateInitializer)
+
+            let initializerProductPromise = self.streamChannelContinuation == nil ? nil : self.channel.eventLoop.makePromise(of: Any.self)


I think it makes more sense to arrange this so that you set up the promise and callback before configuring; you can reduce branching and avoid the '!':

let initializerProductPromise: EventLoopPromise<Any>? if let continuation = self.streamChannelContinuation { let promise = self.channel.eventLoop.makePromise(of: Any.self) promise.futureResult.whenSuccess { ... } initializerProductPromise = promise } else { initializerProductPromise = nil } channel.configureInboundStream(...)

glbrntt · 2023-08-29T08:57:38Z

Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift

+        let anyPromise: EventLoopPromise<Any>?
+        if let promise = promise {
+            anyPromise = channel.baseChannel.eventLoop.makePromise(of: Any.self)
+            promise.completeWith(anyPromise!.futureResult.map { value in value as! Output })


This definitely needs a comment!

I think we can save an allocation here by avoiding a future and completing promise in the whenComplete block of anyPromise:

anyPromise.futureResult.whenComplete { switch $0 { // ... } }

Sources/NIOHTTP2/MultiplexerAbstractChannel.swift

FranzBusch

Overall LGTM! If we just fix up the last comments from @glbrntt

FranzBusch · 2023-09-04T09:20:42Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -989,9 +1004,13 @@ extension NIOHTTP2Handler {
 #if swift(>=5.7)
    /// The type of all `inboundStreamInitializer` callbacks which do not need to return data.
    public typealias StreamInitializer = NIOChannelInitializer
+    /// The type of NIO Channel initializer callbacks which need to return untyped data.
+    internal typealias StreamInitializerWithAnyOutput = @Sendable (Channel) -> EventLoopFuture<Any>


Yeah that makes sense to me

FranzBusch

Just a couple of minor nits left otherwise LGTM!

Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift

FranzBusch · 2023-09-11T08:49:06Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -439,6 +439,7 @@ extension Channel {
    ///   - configuration: The settings that will be used when establishing the connection and new streams.
    ///   - position: The position in the pipeline into which to insert this handler.
    ///   - inboundStreamInitializer: A closure that will be called whenever the remote peer initiates a new stream.
+    ///     It must output the stream `Channel` (either directly or wrapped in some form) to allow users to iterate over inbound streams.


This comment confuses me on all the methods here. The users can return whatever they want inside the initializer right. They might multiplex another HTTP2 connection side a single stream and the return will be a NIOHTTP2Handler.AsyncStreamMultiplexer<NIOHTTP2Handler.AsyncStreamMultiplexer<Output>>>.

Additionally, we should make Output: Sendable here as well.

You're right, this isn't correct, I was attempting to give guidance on the intent of the Output type but it's too narrow a description. I've reworded this to reflect the broader use.

FranzBusch · 2023-09-11T08:49:30Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -537,6 +538,7 @@ extension Channel {
    ///   - http2ConnectionInitializer: An optional callback that will be invoked only when the negotiated protocol
    ///     is HTTP/2 to configure the connection channel.
    ///   - http2InboundStreamInitializer: A closure that will be called whenever the remote peer initiates a new stream.
+    ///     It must output the stream `Channel` (either directly or wrapped in some form) to allow users to iterate over inbound streams.   


We should require all generic types to be Sendable here.

glbrntt

One nit, looks good otherwise.

glbrntt · 2023-09-11T09:47:29Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

@@ -239,7 +239,7 @@ extension NIOHTTP2Handler {
        }

        /// Create a stream channel initialized with the provided closure
-        public func createStreamChannel<OutboundStreamOutput>(_ initializer: @escaping NIOChannelInitializerWithOutput<OutboundStreamOutput>) async throws -> OutboundStreamOutput {
+        public func createStreamChannel<OutboundStreamOutput: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<OutboundStreamOutput>) async throws -> OutboundStreamOutput {


I think OutboundStreamOutput is unnecessarily verbose here: Output is fine.

Agreed. Output should be enough.

I personally still think we should call this method createOutboundStream so that it aligns with the inboundStreamInitializer.

This name aligns with the equivalent method on the non-async multiplexer, I think that alignment is more important.

I personally don't think that is more important because this here is the chance for a new name. I recently saw somebody giving feedback on exactly this API that they are confused what initializer runs when.

I think createOutboundStream is misleading: it sounds like it's a unidirectional stream to me, which it isn't. (FWIW I have the same issue with our inbound stream initializers.)

FranzBusch

LGTM! Thanks @rnro

glbrntt · 2023-09-11T13:39:53Z

These are all SPI changes so are safe to ignore:

14:05:45 5 breaking changes detected in NIOHTTP2:
14:05:45   💔 API breakage: func NIOHTTP2Handler.AsyncStreamMultiplexer.createStreamChannel(_:) has generic signature change from <InboundStreamOutput, OutboundStreamOutput> to <InboundStreamOutput, Output where Output : Swift.Sendable>
14:05:45   💔 API breakage: func Channel.configureAsyncHTTP2Pipeline(mode:configuration:position:inboundStreamInitializer:) has generic signature change from <Self, Output where Self : NIOCore.Channel> to <Self, Output where Self : NIOCore.Channel, Output : Swift.Sendable>
14:05:45   💔 API breakage: func Channel.configureAsyncHTTPServerPipeline(http2Configuration:http1ConnectionInitializer:http2ConnectionInitializer:http2InboundStreamInitializer:) has generic signature change from <Self, HTTP1ConnectionOutput, HTTP2ConnectionOutput, HTTP2StreamOutput where Self : NIOCore.Channel, HTTP1ConnectionOutput : Swift.Sendable> to <Self, HTTP1ConnectionOutput, HTTP2ConnectionOutput, HTTP2StreamOutput where Self : NIOCore.Channel, HTTP1ConnectionOutput : Swift.Sendable, HTTP2ConnectionOutput : Swift.Sendable, HTTP2StreamOutput : Swift.Sendable>
14:05:45   💔 API breakage: func SynchronousOperations.configureAsyncHTTP2Pipeline(mode:configuration:position:inboundStreamInitializer:) has generic signature change from <Output> to <Output where Output : Swift.Sendable>
14:05:45   💔 API breakage: func NIOHTTP2Handler.AsyncStreamMultiplexer.createStreamChannel(configuration:initializer:) has been removed

rnro requested review from glbrntt and FranzBusch August 11, 2023 07:01

FranzBusch reviewed Aug 12, 2023

View reviewed changes

rnro force-pushed the h2_async_initialization branch 4 times, most recently from d275dcc to 69f8db9 Compare August 17, 2023 16:25

rnro requested a review from FranzBusch August 17, 2023 16:31

FranzBusch reviewed Aug 23, 2023

View reviewed changes

rnro added 5 commits August 23, 2023 16:27

Revert "Async stream initializers order matches existing"

e5f3c68

This reverts commit 53be8d4.

concurrency warnings

00e829a

remove async NIOAsyncChannel API

e9d703b

and other review comments

rnro force-pushed the h2_async_initialization branch from 554a6ff to e9d703b Compare August 23, 2023 15:29

rnro requested a review from FranzBusch August 29, 2023 07:33

glbrntt reviewed Aug 29, 2023

View reviewed changes

FranzBusch reviewed Sep 4, 2023

View reviewed changes

review comments

121e221

rnro requested a review from glbrntt September 6, 2023 10:26

rnro force-pushed the h2_async_initialization branch 2 times, most recently from e82124e to 15a6560 Compare September 6, 2023 13:53

fix 5.6

c43f8fd

rnro force-pushed the h2_async_initialization branch from 15a6560 to c43f8fd Compare September 6, 2023 13:55

FranzBusch reviewed Sep 11, 2023

View reviewed changes

glbrntt reviewed Sep 11, 2023

View reviewed changes

review comments

dc80822

FranzBusch approved these changes Sep 11, 2023

View reviewed changes

glbrntt approved these changes Sep 11, 2023

View reviewed changes

glbrntt enabled auto-merge (squash) September 11, 2023 13:40

glbrntt disabled auto-merge September 11, 2023 13:46

glbrntt merged commit 8275314 into apple:main Sep 11, 2023

rnro added the semver/none No version bump required. label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async stream initializers order matches existing #415

Async stream initializers order matches existing #415

rnro commented Aug 11, 2023 •

edited

Loading

FranzBusch Aug 11, 2023

rnro Aug 17, 2023

rnro commented Aug 17, 2023

FranzBusch Aug 23, 2023

rnro Aug 23, 2023

FranzBusch Aug 23, 2023

rnro Aug 23, 2023

FranzBusch Aug 23, 2023

rnro Aug 23, 2023

FranzBusch Aug 23, 2023

rnro Aug 23, 2023

glbrntt Aug 29, 2023

FranzBusch Sep 4, 2023

glbrntt Aug 29, 2023

glbrntt Aug 29, 2023

glbrntt Aug 29, 2023

FranzBusch left a comment

FranzBusch Sep 4, 2023

FranzBusch left a comment

FranzBusch Sep 11, 2023

rnro Sep 11, 2023

FranzBusch Sep 11, 2023

glbrntt left a comment

glbrntt Sep 11, 2023

FranzBusch Sep 11, 2023

FranzBusch Sep 11, 2023

rnro Sep 11, 2023

FranzBusch Sep 11, 2023

glbrntt Sep 11, 2023

FranzBusch left a comment

glbrntt commented Sep 11, 2023

Async stream initializers order matches existing #415

Async stream initializers order matches existing #415

Conversation

rnro commented Aug 11, 2023 • edited Loading

Motivation:

Modifications:

Result:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro commented Aug 17, 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

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch 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

glbrntt 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

FranzBusch left a comment

Choose a reason for hiding this comment

glbrntt commented Sep 11, 2023

rnro commented Aug 11, 2023 •

edited

Loading