Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make HTTP2StreamChannel generic over a message type #218

Merged
merged 2 commits into from
Jul 29, 2020

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jul 28, 2020

Motivation:

As part of #214 we need to make HTTP2StreamChannel generic over the type
of message it expects to read and write. This allows us to define an
HTTP2StreamChannel which uses HTTP2Frame.FramePayload as its
currency type, rather than HTTP2Frame.

Modifications:

  • Make HTTP2StreamChannel generic over Message which is constrained
    by conformance to HTTP2FrameConvertible and
    HTTP2FramePayloadConvertible
  • HTTP2StreamChannel now expects to read in Messages (as opposed to
    HTTP2Frames) and have Messages written in to it.
  • Added typealiases for frame and payload based stream channels.
  • Added support for the payload based channel in
    MultiplexerAbstractChannel (although one can't be created yet).

Result:

We can create payload based stream channels.

@glbrntt glbrntt added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jul 28, 2020
@glbrntt glbrntt requested a review from Lukasa July 28, 2020 15:10
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly looks good, I have a few notes.

Sources/NIOHTTP2/HTTP2FrameConvertible.swift Show resolved Hide resolved
@@ -615,7 +621,7 @@ internal extension HTTP2StreamChannel {
///
/// - parameters:
/// - frame: The `HTTP2Frame` received from the network.
func receiveInboundFrame(_ frame: HTTP2Frame) {
func receiveInbound(_ message: Message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather that this didn't change, and instead that we added a line to convert the frame to Message in this method.

@@ -77,62 +84,80 @@ extension MultiplexerAbstractChannel {
switch self.baseChannel {
case .frameBased(let base):
base.configure(initializer: initializer, userPromise: promise)
case .payloadBased(let base):
base.configure(initializer: initializer, userPromise: promise)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fatalError here: we don't ever want this branch to be hit.

Motivation:

As part of apple#214 we need to make `HTTP2StreamChannel` generic over the type
of message it expects to read and write. This allows us to define an
`HTTP2StreamChannel` which uses `HTTP2Frame.FramePayload` as its
currency type, rather than `HTTP2Frame`.

Modifications:

- Make `HTTP2StreamChannel` generic over `Message` which is constrained
  by conformance to `HTTP2FrameConvertible` and
  `HTTP2FramePayloadConvertible`
- `HTTP2StreamChannel` now expects to read in `Message`s (as opposed to
  `HTTP2Frame`s) and have `Message`s written in to it.
- Added typealiases for frame and payload based stream channels.
- Added support for the payload based channel in
  `MultiplexerAbstractChannel` (although one can't be created yet).

Result:

We can create payload based stream channels.
@glbrntt glbrntt requested a review from Lukasa July 28, 2020 16:52
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@Lukasa Lukasa merged commit cde820e into apple:master Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants