HTTP/2 Async API #424
Merged
HTTP/2 Async API #424
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FranzBusch
reviewed
Oct 25, 2023
Comment on lines
226
to
227
| /// Outbound stream channel objects are initialized upon creation using the supplied `streamStateInitializer` which returns a type | ||
| /// `Output`. This type may be `HTTP2Frame` or changed to any other type. |
There was a problem hiding this comment.
Let's rephrase that to talk about opening new streams to a remote and uptime the streamStateInitializer
| } | ||
|
|
||
| /// Create a stream channel initialized with the provided closure | ||
| public func openStream<Output: Sendable>(_ initializer: @escaping NIOChannelInitializerWithOutput<Output>) async throws -> Output { |
There was a problem hiding this comment.
Can we document the initializer closure here please
| /// `NIOHTTP2InboundStreamChannels` provides access to inbound stream channels as a generic `AsyncSequence`. | ||
| /// They make use of generics to allow for wrapping the stream `Channel`s, for example as `NIOAsyncChannel`s or protocol negotiation objects. | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| public struct NIOHTTP2InboundStreamChannels<Output>: AsyncSequence { |
There was a problem hiding this comment.
Should we nest this type inside the AsyncStreamMultiplexer ?
There was a problem hiding this comment.
I don't have a strong opinion here but I'm not convinced. I don't know if this type has to be tightly coupled to the async multiplexer and since this is public API maybe we should keep our options open?
Comment on lines
456
to
457
| /// `NIOHTTP2InboundStreamChannels` provides access to inbound stream channels as a generic `AsyncSequence`. | ||
| /// They make use of generics to allow for wrapping the stream `Channel`s, for example as `NIOAsyncChannel`s or protocol negotiation objects. |
There was a problem hiding this comment.
Should we drop inbound here?
| } | ||
| } | ||
|
|
||
| #if swift(>=5.7) |
There was a problem hiding this comment.
We are 5.7+ only right? So we can drop those guards
| // this should be available in the std lib from 5.9 onwards | ||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| extension AsyncThrowingStream { | ||
| public static func makeStream( |
There was a problem hiding this comment.
This shouldn't be public right?
Comment on lines
609
to
610
| case http1_1(HTTP1Output) | ||
| case http2(HTTP2Output) |
There was a problem hiding this comment.
Can we document the two cases please
FranzBusch
reviewed
Oct 25, 2023
| /// atom, as opposed to the regular NIO `SelectableChannel` objects which use `ByteBuffer` | ||
| /// and `IOData`. | ||
| /// | ||
| /// Locally-initiated stream channel objects are initialized upon creation using the supplied `initializer` which returns a type |
There was a problem hiding this comment.
This is hinting at the openStream method right? We should also talk about remote initiated stream here which use the InboundStreamOutput generic type (maybe we should rename that type?)
There was a problem hiding this comment.
We can also explicitly spell out how the users can open streams. (i.e. "You can open a stream by calling ....")
glbrntt
reviewed
Oct 25, 2023
| /// atom, as opposed to the regular NIO `SelectableChannel` objects which use `ByteBuffer` | ||
| /// and `IOData`. | ||
| /// | ||
| /// Locally-initiated stream channel objects are initialized upon creation using the supplied `initializer` which returns a type |
There was a problem hiding this comment.
We can also explicitly spell out how the users can open streams. (i.e. "You can open a stream by calling ....")
| /// Create a stream channel initialized with the provided closure | ||
| /// - Parameter initializer: A closure that will be called upon the created stream which is responsible for | ||
| /// initializing the stream's `Channel`. | ||
| /// - Returns: The optionally-wrapped initialized channel. |
There was a problem hiding this comment.
I think this is really the result of the initializer, doesn't have to be the channel at all
| @@ -451,3 +451,114 @@ internal protocol AnyContinuation { | |||
| func finish() | |||
| func finish(throwing error: Error) | |||
| } | |||
|
|
|||
|
|
|||
| /// `NIOHTTP2StreamChannels` provides access to stream channels as a generic `AsyncSequence`. | |||
There was a problem hiding this comment.
This needs updating: not necessarily stream channels.
| private var continuation: AsyncThrowingStream<Output, Error>.Continuation | ||
|
|
||
| internal init( | ||
| continuation: AsyncThrowingStream<Output, Error>.Continuation |
There was a problem hiding this comment.
I tend to use wrapping as the label in the init for wrapper types. It makes it a little more obvious that the type really is just a simple wrapper.
glbrntt
approved these changes
Oct 25, 2023
FranzBusch
approved these changes
Oct 25, 2023
|
This pull broke the tests on Android and linux, as first surfaced by my daily Android CI yesterday: The reason it probably passed CI is because it appears to be some kind of race, as it is not always reproducible. I can fairly consistently reproduce on slower hardware though, where passing in |
|
@FranzBusch can you take a look at this? |
|
@finagolfin I just ran the |
|
Here's the full error output from a single-core linux x86_64 VPS, with this repo checked out to this commit 462ded7: |
|
Thanks for the backtrace @finagolfin. Just put up a PR to fix this: apple/swift-nio#2580 |
|
@finagolfin release with the fix is out: https://github.com/apple/swift-nio/releases/tag/2.61.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We previously developed an async API for NIO HTTP/2 which was guarded under SPI. Now that the swift-nio async API is released we can reintroduce this code promoted to SPI.
This change introduces:
AsyncStreamMultiplexer- an async variant of the HTTP/2 stream multiplexer which can be used to create outbound streams and provide access to an async sequence (NIOHTTP2AsyncSequence) of inbound streamsconfigureAsyncHTTP2Pipeline) to support the new async mode