Add docs for the async NIO APIs #2549

FranzBusch · 2023-10-11T16:53:27Z

Motivation

To ease reviewing of all the new APIs and to document some of the thought process behind them, I threw together a document that collects all of it.

We can use this document to holistcally review the APIs now before removing the SPI.

docs/public-async-nio-apis.md

FranzBusch · 2023-10-12T10:02:03Z

docs/public-async-nio-apis.md

+    public func configureAsyncHTTP2Pipeline<Output: Sendable>(
+        mode: NIOHTTP2Handler.ParserMode,
+        configuration: NIOHTTP2Handler.Configuration = .init(),
+        position: ChannelPipeline.Position = .last,


I think we should drop the position here because it is a type we cannot make Sendable

We almost certainly have to drop it: semantically the only place we can do this is last. What does it mean to add your async channel handlers not last?

I think it's fine to drop it. The only times when it might be used I can think of are incredibly contrived and it's possible to work around them anyway. So, yeah I'm in favor of dropping the non-sendable footgun.

docs/public-async-nio-apis.md

FranzBusch · 2023-10-12T10:03:05Z

docs/public-async-nio-apis.md

+    ///     be waited on to retrieve the result of the negotiation.
+    @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+    public func configureAsyncHTTPServerPipeline<HTTP1ConnectionOutput: Sendable, HTTP2ConnectionOutput: Sendable, HTTP2StreamOutput: Sendable>(
+        http2Configuration: NIOHTTP2Handler.Configuration = .init(),


What about the HTTP2 parser mode here @rnro?

This is the function for configuring a server pipeline so the parser mode is hardcoded to .server

FranzBusch · 2023-10-12T10:13:08Z

docs/public-async-nio-apis.md

+    @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+    public struct AsyncStreamMultiplexer<InboundStreamOutput> {
+        /// Create a stream channel initialized with the provided closure
+        public func createStreamChannel<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output


I am still wondering if we should call this createOutboundStream this would provide a good balance to the InboundStreamOutput e.g. cc @rnro

Suggested change

public func createStreamChannel<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output

public func createOutboundStream<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output

If we are dropping consistency with the old multiplexer then I think createOutboundStream is an improvement, but I'm now actually wondering if we should just go for createStream, do we need to disambiguate against somehow creating an inbound stream? cc @glbrntt

There's still something which feels a bit vague about how we call createStream/createOutboundStream but then hand back the caller basically anything. Our assumption is that they wrapped the channel in the initializer and returned that, which might be fair enough but I'm not sure that the word initializer conveys all that (new to async-land) responsibility.

createStreamChannel is slightly odd because we don't return a Channel

createOutboundStream is better, but it's not possible to create inbound streams so I don't think the "outbound" distinction is important

createStream is better still

There's still something which feels a bit vague about how we call createStream/createOutboundStream but then hand back the caller basically anything. Our assumption is that they wrapped the channel in the initializer and returned that, which might be fair enough but I'm not sure that the word initializer conveys all that (new to async-land) responsibility.

This is true but I don't know if it's really an issue; regardless of what the user does we're still creating a stream. I think initialiser is fine in this context though.

paging @dnadoba who usually has great instincts with naming...

We're assuming inboundStreamIntializer is the right name, which isn't necessarily the case (although it matters much less IMO because trailing closure syntax). Maybe it should also just be initializer.

IMO naming it just initializer is a step back. We just had recent feedback in the forums that a users was confused about what that initializer does. So become more vague feels like a step in the wrong direction.

We just had recent feedback in the forums that a users was confused about what that initializer does.

Can you share a link to this?

Neither inboundStreamIntializer nor initializer will tell you what an initialiser does. inboundStream tells you what the initialiser applies to, without inboundStream you assume from the context what it applies to.

The comment was in the Swift Open Source Slack actually and not in the forums. This was the users feedback

update: i finally got an HTTP/2 request working, the piece i was missing was that you still need to add your application stream handler in NIOHTTP2Handler.StreamMultiplexer.createStreamChannel(_:), even if you already provided a callback when setting up the handlers with Channel.configureHTTP2Pipeline. which was very counterintuitive given how the docs are worded:
https://swiftinit.org/docs/swift-nio-http2/niohttp2/niohttp2handler.multiplexer
it gave the impression that if you provide the callback during outer setup then the multiplexer is “ready to go” and you can just return $0.eventLoop.createSucceededVoidFuture(). please update these docs 🙂

To be fair it is more about the documentation explaining the different of the two closures and why both are needed.

Personally, I think there is still value in having outbound in the method name.

Yeah that's absolutely about the docs and not about the naming. The linked docs do call out that it's an outbound stream which wasn't sufficient so I'm not sure keeping it in the name makes a difference. That said, this isn't a hill I'll die on!

docs/public-async-nio-apis.md

glbrntt · 2023-10-12T12:12:44Z

docs/public-async-nio-apis.md

+    public let inboundStream: NIOCore.NIOAsyncChannelInboundStream<Inbound>
+
+    /// The writer for writing outbound messages.
+    public let outboundWriter: NIOCore.NIOAsyncChannelOutboundWriter<Outbound>


Not sure about naming here: both "stream" and "writer" are in the type names. Would prefer inbound and outbound.

I am not yet convinced. It feels a bit magical that users should know that inbound implies stream and outbound implies writer. Could be the other way around as well.

It's not difficult to discover though, both the type and the documentation are present to help users know what to do with them.

I have a pretty strong aversion to including type names in property names... and I don't think there are many places where you'd be happy with widgetsArray as a name vs widgets.

I think I'm with George FWIW. The fact that the things which offer inbound and outbound functionality are different types shouldn't be a surprise to anyone, and they will need to check the types to see how to interact with them anyway.

docs/public-async-nio-apis.md

Lukasa · 2023-10-13T09:38:49Z

docs/public-async-nio-apis.md

+    public func configureAsyncHTTP2Pipeline<Output: Sendable>(
+        mode: NIOHTTP2Handler.ParserMode,
+        configuration: NIOHTTP2Handler.Configuration = .init(),
+        position: ChannelPipeline.Position = .last,


We almost certainly have to drop it: semantically the only place we can do this is last. What does it mean to add your async channel handlers not last?

rnro · 2023-10-13T14:29:03Z

docs/public-async-nio-apis.md

+handlers which bridge the read and write side using the
+`NIOAsyncSequenceProducer` and the `NIOAsyncWriter`.
+
+Next up we had to look at the bootstraps. Here the import part is that the


Suggested change

Next up we had to look at the bootstraps. Here the import part is that the

Next we had to look at the bootstraps. Here the important part was recognizing that the

docs/public-async-nio-apis.md

# Motivation To ease reviewing of all the new APIs and to document some of the thought process behind them, I threw together a document that collects all of it. We can use this document to holistcally review the APIs now before removing the SPI.

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

FranzBusch commented Oct 12, 2023

View reviewed changes

glbrntt reviewed Oct 12, 2023

View reviewed changes

FranzBusch requested review from glbrntt and Lukasa October 12, 2023 16:34

glbrntt approved these changes Oct 13, 2023

View reviewed changes

Lukasa reviewed Oct 13, 2023

View reviewed changes

FranzBusch force-pushed the fb-async-nio-api branch from 5408154 to 186c789 Compare October 13, 2023 12:24

FranzBusch requested a review from Lukasa October 13, 2023 12:25

rnro reviewed Oct 13, 2023

View reviewed changes

docs/public-async-nio-apis.md Outdated Show resolved Hide resolved

rnro reviewed Oct 13, 2023

View reviewed changes

docs/public-async-nio-apis.md Outdated Show resolved Hide resolved

rnro reviewed Oct 13, 2023

View reviewed changes

docs/public-async-nio-apis.md Outdated Show resolved Hide resolved

rnro reviewed Oct 13, 2023

View reviewed changes

docs/public-async-nio-apis.md Outdated Show resolved Hide resolved

FranzBusch added 9 commits October 16, 2023 13:38

Add docs for the async NIO APIs

0a5ec73

# Motivation To ease reviewing of all the new APIs and to document some of the thought process behind them, I threw together a document that collects all of it. We can use this document to holistcally review the APIs now before removing the SPI.

Franz review

cae33d2

George review

fa6692f

Rename inboundStream and outboundWriter

832e9ca

Make NIOTypedHTTPClientUpgradeHandler method private

1ec2487

Cory review

f08654f

Remove position from API

7d34995

Rick review

4b99220

Remove NIOProtocolNegotationResult from API

a49804e

FranzBusch force-pushed the fb-async-nio-api branch from f7a8a21 to a49804e Compare October 16, 2023 12:41

Fix benchmark

a9dc7cf

FranzBusch added 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 and removed needs-no-version-bump For PRs that when merged do not need a bump in version number. labels Oct 16, 2023

glbrntt merged commit 0fb8cb7 into apple:main Oct 16, 2023
7 of 8 checks passed

FranzBusch deleted the fb-async-nio-api branch October 16, 2023 14:03

FranzBusch mentioned this pull request Oct 16, 2023

Remove SPI from NIOAsyncChannel and new bootstrap methods #2548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docs for the async NIO APIs #2549

Add docs for the async NIO APIs #2549

FranzBusch commented Oct 11, 2023

FranzBusch Oct 12, 2023

FranzBusch Oct 12, 2023

Lukasa Oct 13, 2023

rnro Oct 13, 2023

FranzBusch Oct 12, 2023

rnro Oct 13, 2023

FranzBusch Oct 12, 2023

rnro Oct 13, 2023

rnro Oct 13, 2023

glbrntt Oct 13, 2023 •

edited

Loading

glbrntt Oct 13, 2023

glbrntt Oct 13, 2023

FranzBusch Oct 13, 2023

glbrntt Oct 13, 2023

FranzBusch Oct 13, 2023

glbrntt Oct 13, 2023

glbrntt Oct 12, 2023

FranzBusch Oct 12, 2023

glbrntt Oct 12, 2023

rnro Oct 13, 2023 •

edited

Loading

Lukasa Oct 13, 2023

rnro Oct 13, 2023

	public func createStreamChannel<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output
	public func createOutboundStream<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output

	Next up we had to look at the bootstraps. Here the import part is that the
	Next we had to look at the bootstraps. Here the important part was recognizing that the

Add docs for the async NIO APIs #2549

Add docs for the async NIO APIs #2549

Conversation

FranzBusch commented Oct 11, 2023

Motivation

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 Oct 13, 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

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

rnro Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt Oct 13, 2023 •

edited

Loading

rnro Oct 13, 2023 •

edited

Loading