Add `AsyncChannel` based `ServerBootstrap.bind()` methods #2403

FranzBusch · 2023-04-12T15:49:42Z

Motivation

In my previous PR, we added a new async bridge from a NIO Channel to Swift Concurrency primitives in the from of the NIOAsyncChannel. This type alone is already helpful in bridging Channels to Concurrency; however, it is hard to use since it requires to wrap the Channel at the right time otherwise we will drop reads. Furthermore, in the case of protocol negotiation this becomes even trickier since we need to wait until it finishes and then wrap the Channel.

Modification

This PR introduces a few things:

New methods on the ServerBootstrap which allow the creation of NIOAsyncChannel based channels. This can be used in all cases where no protocol negotiation is involved.
A new protocol and type called NIOProtocolNegotiationHandler and NIOProtocolNegotiationResult which is used to identify channel handlers that are doing protocol negotiation.
New methods on the ServerBootstrap that are aware of protocol negotiation.

Result

We can now easily and safely create new AsyncChannels from the ServerBootstrap

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift

Sources/NIOPosix/Bootstrap.swift

glbrntt · 2023-04-13T09:47:32Z

Sources/NIOPosix/Bootstrap.swift

+        }.flatMap {
+            $0


I think if you use eventLoop.flatSubmit you can drop this

Where would I use the flatSubmit here?

Instead of the submit on L699

Sources/NIOPosix/Bootstrap.swift

glbrntt · 2023-04-13T09:50:44Z

Sources/NIOPosix/Bootstrap.swift

+        }.flatMap {
+            $0


same here re: flat submit

Sources/NIOPosix/Bootstrap.swift

glbrntt · 2023-04-13T09:58:04Z

Sources/NIOTLS/NIOTypedApplicationProtocolNegotiationHandler.swift

+        let switchFuture = self.completionHandler(result, context.channel)
+        switchFuture
+            .whenComplete { result in
+                // We must be in the event loop here to make sure no hops have happened


Is this an API requirement? Can't we just hop back to the right event-loop?

It is a tricky one. In theory, yes it is fine to hop here since the user can add the handlers to the pipeline in their closure and then hop to another future.
I am going to remove that preconditions. Users can screw up with weird construction anyhow but hopefully they don't have to wrap into NIOAsyncChannel themselves often and can just use our configureXXX methods.

Sources/NIOPosix/Bootstrap.swift

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStream.swift

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift

gjcairo · 2023-04-13T15:35:17Z

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift

+    ) where InboundIn == ProducerElement {
+        self.eventLoop = eventLoop
+        self.closeRatchet = closeRatchet
+        self.transformation = .sync { $0 }


Would it make sense to have transformation be an optional instead of having to have this identity transformation here? Maybe the compiler sees through this and optimises it, but if not, I'm thinking having to call transformation(unwrapped) on each channelRead could cause some unnecessary overhead when there's no actual transformation to be done.

I tried this initially but this doesn't work because the compiler can't infer that if the transformation is none that InboundIn == ProducerElement. The compiler should be smart enough here though to see that the closure is a no-op in this case and be able to optimise it away.

There's no way the compiler makes that optimization, but it's ok, the identity function is going to be pretty cheap, and this is an implementation detail we can fix.

gjcairo · 2023-04-14T08:45:00Z

Sources/NIOPosix/Bootstrap.swift

+    /// Bind the `ServerSocketChannel` to the `unixDomainSocketPath` parameter.
+    ///
+    /// - Parameters:
+    ///   - unixDomainSocketPath: The _Unix domain socket_ path to bind to. `unixDomainSocketPath` must not exist, it will be created by the system.


unixDomainSocketPath must not exist, it will be created by the system.

I don't think this applies to this version of the init, as you can set the cleanupExistingSocketFile flag to clean up the socket if it exists already, correct?

Sources/NIOPosix/Bootstrap.swift

Sources/NIOTLS/NIOTypedApplicationProtocolNegotiationHandler.swift

Lukasa · 2023-04-18T10:07:58Z

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

+        synchronouslyWrapping channel: Channel,
+        backpressureStrategy: NIOAsyncSequenceProducerBackPressureStrategies.HighLowWatermark? = nil,
+        isOutboundHalfClosureEnabled: Bool = false,
+        protocolNegotiationClosure: @escaping (Channel) -> EventLoopFuture<Inbound>


I'm a bit nervous about having these two methods differ only on the label of a trailing closure, which is commonly omitted.

My plan was to keep these methods spi forever and never make them truly public since we only really use them from the bootstrap. Does that make sense to you and remove some of the nervousness?

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStream.swift

Lukasa · 2023-04-18T10:14:13Z

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift

+    ) where InboundIn == ProducerElement {
+        self.eventLoop = eventLoop
+        self.closeRatchet = closeRatchet
+        self.transformation = .sync { $0 }


There's no way the compiler makes that optimization, but it's ok, the identity function is going to be pretty cheap, and this is an implementation detail we can fix.

Sources/NIOTLS/NIOTypedApplicationProtocolNegotiationHandler.swift

adam-fowler · 2023-04-19T14:16:10Z

Sources/NIOPosix/Bootstrap.swift

+                        synchronouslyWrapping: serverChannel,
+                        backpressureStrategy: serverBackpressureStrategy,
+                        transformationClosure: { channel in
+                            try NIOAsyncChannel(


It appears this is being called on the server channel eventloop, but it expects to be called on the child channel eventloop.

Thanks. Good catch!

You should run your tests with an EventLoopGroup with more than one thread or at least have one test with this. Three is a good number to ensure you don't have a server and a client interleaving EventLoop usage. I was running with two threads and only caught this because I had a test which had two client requests in it which opened two child channels

This is a very good piece of advice. I also recommend adding preconditionInEventLoop checks for any code that expects it, because they're fairly cheap and they flush out these bugs really early.

Yeah the preconditions were already in place and our tests caught this once I added more than on thread to the ELG. I also just pushed some commits that refactors the bootstrapping more to make all of this properly work.

# Motivation In my previous PR, we added a new async bridge from a NIO `Channel` to Swift Concurrency primitives in the from of the `NIOAsyncChannel`. This type alone is already helpful in bridging `Channel`s to Concurrency; however, it is hard to use since it requires to wrap the `Channel` at the right time otherwise we will drop reads. Furthermore, in the case of protocol negotiation this becomes even trickier since we need to wait until it finishes and then wrap the `Channel`. # Modification This PR introduces a few things: 1. New methods on the `ServerBootstrap` which allow the creation of `NIOAsyncChannel` based channels. This can be used in all cases where no protocol negotiation is involved. 2. A new protocol and type called `NIOProtocolNegotiationHandler` and `NIOProtocolNegotiationResult` which is used to identify channel handlers that are doing protocol negotiation. 3. New methods on the `ServerBootstrap` that are aware of protocol negotiation. # Result We can now easily and safely create new `AsyncChannel`s from the `ServerBootstrap`

Lukasa

I've left some notes in the diff.

I also have a broader structural question I want us to think about: is searching for the handler by type the right thing to do here? This forces a lot of runtime type checking in more complex pipelines, which may not be the right thing to do.

Our other options are:

Search by name. It's a little-known feature of NIO that handlers can be named, and can be searched for by name. This is a little cheaper, and might help us.
Require that the handler be passed to the bind call. This is tricky, because most handlers aren't Sendable.
Update the childChannelInitializer to require that the user returns the negotiation handler, instead of Void.

Do you have thoughts here?

Sources/NIOCore/AsyncChannel/AsyncChannelInboundStreamChannelHandler.swift

Tests/NIOPosixTests/AsyncChannelBootstrapTests+XCTest.swift

Tests/NIOTLSTests/NIOTypedApplicationProtocolNegotiationHandlerTests+XCTest.swift

Tests/NIOTLSTests/NIOTypedApplicationProtocolNegotiationHandlerTests.swift

Lukasa · 2023-04-26T09:41:37Z

Sources/NIOPosix/Bootstrap.swift

+    }
+
+    private typealias MakeServerChannel = (_ eventLoop: SelectableEventLoop, _ childGroup: EventLoopGroup, _ enableMPTCP: Bool) throws -> ServerSocketChannel
+    private typealias Register = (EventLoop, ServerSocketChannel) -> EventLoopFuture<Void>


Do either of these need @Sendable?

Yes Register needs to Sendable since it is run on the server socket EL. However, MakeServerChannel is run inline and not escaping.

FranzBusch · 2023-04-26T12:46:42Z

Our other options are:

Search by name. It's a little-known feature of NIO that handlers can be named, and can be searched for by name. This is a little cheaper, and might help us.

Require that the handler be passed to the bind call. This is tricky, because most handlers aren't Sendable.

Update the childChannelInitializer to require that the user returns the negotiation handler, instead of Void.

Do you have thoughts here?

Good question!

I don't think searching by name works here. The problem is that we need the fully typed generic handler. We often expect to have something like a NIOTypedApplicationProtocolNegotiationHandler<Something> and when we search by name we would still need to cast it. Potentially we can only do this dynamic casting once instead of for every handler.
Yeah the not Sendability of handlers doesn't really make this possible. That's why I ruled this out earlier.
I choose not to do this because it would require some larger changes in how we do the bootstrap since we have to tie together the return type of the childChannelInitializer to the type we expect to return from bind. We can only do this by introducing a new bootstrap type which I tried to avoid so far. However, we can definitely explore this and it would remove the runtime checks.

WDYT?

Lukasa · 2023-04-26T13:34:34Z

I wonder if it's a good time for us to explore (3). However, as this is SPI we can merge this without that change and investigate it later.

FranzBusch · 2023-04-26T13:40:19Z

I wonder if it's a good time for us to explore (3). However, as this is SPI we can merge this without that change and investigate it later.

I would prefer to merge this now to unblock the HTTP2 work and will track this as an open item before we make the APIs public

FranzBusch force-pushed the fb-async-channel-bootstrap branch from f2e4d05 to 7f2a1de Compare April 12, 2023 15:51

glbrntt reviewed Apr 13, 2023

View reviewed changes

FranzBusch force-pushed the fb-async-channel-bootstrap branch from 7f2a1de to e8795a0 Compare April 13, 2023 12:11

FranzBusch marked this pull request as ready for review April 13, 2023 12:52

FranzBusch requested review from Lukasa and glbrntt April 13, 2023 12:52

glbrntt reviewed Apr 13, 2023

View reviewed changes

FranzBusch requested a review from glbrntt April 14, 2023 09:25

gjcairo reviewed Apr 14, 2023

View reviewed changes

Lukasa reviewed Apr 18, 2023

View reviewed changes

FranzBusch force-pushed the fb-async-channel-bootstrap branch from 0ccfa6f to 09941a4 Compare April 19, 2023 09:16

FranzBusch requested review from Lukasa and gjcairo April 19, 2023 10:02

adam-fowler reviewed Apr 19, 2023

View reviewed changes

FranzBusch force-pushed the fb-async-channel-bootstrap branch from 95fb851 to 53d553c Compare April 24, 2023 11:18

FranzBusch added 10 commits April 24, 2023 14:23

Code review

53723f7

Fix typo

fe7c3dc

Fix up tests

f9785df

Stop finishing the writer when an error is caught

f5badb9

Code review

5278e08

Fix up writer tests

4eccf82

Introduce shared protocol negotiation handler state machine

cb8fff7

Correctly handle multi threaded event loops

69c4961

Adapt test to assert the channel was closed correctly.

0f91297

FranzBusch force-pushed the fb-async-channel-bootstrap branch from 53d553c to 0f91297 Compare April 24, 2023 13:23

Lukasa requested changes Apr 26, 2023

View reviewed changes

Code review

5da699d

Lukasa added the semver/none No version bump required. label Apr 26, 2023

Lukasa approved these changes Apr 26, 2023

View reviewed changes

FranzBusch enabled auto-merge (squash) April 26, 2023 13:51

FranzBusch merged commit d836d6b into apple:main Apr 26, 2023

FranzBusch deleted the fb-async-channel-bootstrap branch April 26, 2023 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `AsyncChannel` based `ServerBootstrap.bind()` methods #2403

Add `AsyncChannel` based `ServerBootstrap.bind()` methods #2403

FranzBusch commented Apr 12, 2023

glbrntt Apr 13, 2023

FranzBusch Apr 13, 2023

glbrntt Apr 13, 2023

glbrntt Apr 13, 2023

glbrntt Apr 13, 2023

FranzBusch Apr 13, 2023

gjcairo Apr 13, 2023

FranzBusch Apr 18, 2023

Lukasa Apr 18, 2023

gjcairo Apr 14, 2023

Lukasa Apr 18, 2023

FranzBusch Apr 18, 2023

Lukasa Apr 18, 2023

adam-fowler Apr 19, 2023 •

edited

Loading

FranzBusch Apr 19, 2023

adam-fowler Apr 19, 2023

Lukasa Apr 20, 2023

FranzBusch Apr 24, 2023

Lukasa left a comment

Lukasa Apr 26, 2023

FranzBusch Apr 26, 2023

FranzBusch commented Apr 26, 2023

Lukasa commented Apr 26, 2023

FranzBusch commented Apr 26, 2023

Add AsyncChannel based ServerBootstrap.bind() methods #2403

Add AsyncChannel based ServerBootstrap.bind() methods #2403

Conversation

FranzBusch commented Apr 12, 2023

Motivation

Modification

Result

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

adam-fowler Apr 19, 2023 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch commented Apr 26, 2023

Lukasa commented Apr 26, 2023

FranzBusch commented Apr 26, 2023

Add `AsyncChannel` based `ServerBootstrap.bind()` methods #2403

Add `AsyncChannel` based `ServerBootstrap.bind()` methods #2403

adam-fowler Apr 19, 2023 •

edited

Loading