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.

Sign up for GitHub

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

Jump to bottom

Allow payload-based stream channels to be created. #221

Merged
merged 3 commits into from Aug 3, 2020
Merged

Allow payload-based stream channels to be created. #221

merged 3 commits into from Aug 3, 2020

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jul 29, 2020

Motivation:

To lift the requirement that stream creation order must match the order
of the first write on those channels we added a payload-based stream
channel. We need a way for users to create these channels and initialze
these channels when they are inbound.

Modifications:

  • Adds a createStreamChannel using a stream initialzer without a
    stream ID
  • Adds a new multiplexer init where the inbound stream initialzer does
    not use stream IDs
  • Adds an additional 'configure' to MultiplexerAbstractChannel
  • Replaces the init in MultiplexerAbstractChannel with two static funcs:
    we can't switch on an optional stream ID since inbound streams always
    have a stream ID; we need to be be able to explicitly support choosing
    the type of channel we want.
  • Minor refactoring in HTTP2StreamChannel to avoid duplication between the
    configure functions

Result:

  • The multiplexer can create payload based streams manually and when a
    new stream is inbound.

@glbrntt glbrntt requested a review from Lukasa July 29, 2020 13:33
@glbrntt glbrntt added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Jul 29, 2020
@glbrntt
Copy link
Contributor Author

glbrntt commented Jul 29, 2020

Hmm allocations have increased for create_client_stream_channel on 5.0... 👀

Lukasa
Lukasa reviewed Jul 29, 2020
View reviewed changes
Sources/NIOHTTP2/HTTP2StreamChannel.swift Outdated Show resolved Hide resolved
Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift Outdated
)
self.streams[streamID] = channel
channel.configure(initializer: self.inboundStreamStateInitializer, userPromise: nil)
let channel: MultiplexerAbstractChannel
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we arrange for MultiplexerAbstractChannel to paper over this distinction? I think we can do it if we make the enum for inboundStreamStateInitializer something we pass to the MultiplexerAbstractChannel and have it end up choosing which base channel to construct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's sensible. Done.

@Lukasa Lukasa mentioned this pull request Jul 29, 2020
Closed
8 tasks
Lukasa
Lukasa reviewed Jul 29, 2020
View reviewed changes
Sources/NIOHTTP2/MultiplexerAbstractChannel.swift Outdated
@@ -25,21 +25,42 @@ import NIO
/// The implementation of `Equatable` & `Hashable` on this type reinforces that requirement.
struct MultiplexerAbstractChannel {
private var baseChannel: BaseChannel
private var inboundStreamStateInitializer: InboundStreamStateInitializer

private init(baseChannel: BaseChannel, inboundStreamStateInitializer: InboundStreamStateInitializer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this isn't quite what I wanted either. I wanted to pass the initializer to the relevant functions. This forces us to store the inbound stream state initializer on all streams, which is mostly extra work for us. Any reason not to just pass it to configureInboundStream?

We don't need to store this function: we just need to have it in the init to know what channel to create. That's totally sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha.

@glbrntt
Allow payload-based stream channels to be created.
30e4d8d 
Motivation:

To lift the requirement that stream creation order must match the order
of the first write on those channels we added a payload-based stream
channel. We need a way for users to create these channels and initialze
these channels when they are inbound.

Modifications:

- Adds a `createStreamChannel` using a stream initialzer without a
  stream ID
- Adds a new multiplexer `init` where the inbound stream initialzer does
  not use stream IDs
- Adds an additional 'configure' to MultiplexerAbstractChannel
- Replaces the `init` in `MultiplexerAbstractChannel` with two `static func`s:
  we can't switch on an optional stream ID since inbound streams always
  have a stream ID; we need to be be able to explicitly support choosing
  the type of channel we want.
- Minor refactoring in `HTTP2StreamChannel` to avoid duplication between the
  `configure` functions

Result:

- The multiplexer can create payload based streams manually and when a
  new stream is inbound.
glbrntt added a commit to glbrntt/swift-nio-http2 that referenced this pull request Jul 30, 2020
@glbrntt
Add HTTP2Frame.FramePayload codecs
37ca6c7 
Motivation:

As part of the work for apple#214 we need new codecs which transform
`HTTP2Frame.FramePayload` to and from the appropriate request and
response parts.

Modifications:

- Add `HTTP2FramePayloadToHTTP1ClientCodec`
- Add `HTTP2FramePayloadToHTTP1ServerCodec`
- Duplicate the HTTP2ToHTTP1CodecTests and update the relevant parts to
  use payloads instead of frames.
- Add relevant test helpers.
- Note: the HTTP2 to HTTP1 (frame based) codecs haven't been deprecated:
  doing so without warnings depends on apple#221.

Result:

- We can transform `HTTP2Frame.FramePayload` types to the relevant HTTP
  client and server request and response types.
glbrntt added a commit to glbrntt/swift-nio-http2 that referenced this pull request Jul 30, 2020
@glbrntt
Add HTTP2Frame.FramePayload codecs
64c5631 
Motivation:

As part of the work for apple#214 we need new codecs which transform
`HTTP2Frame.FramePayload` to and from the appropriate request and
response parts.

Modifications:

- Add `HTTP2FramePayloadToHTTP1ClientCodec`
- Add `HTTP2FramePayloadToHTTP1ServerCodec`
- Duplicate the HTTP2ToHTTP1CodecTests and update the relevant parts to
  use payloads instead of frames.
- Add relevant test helpers.
- Note: the HTTP2 to HTTP1 (frame based) codecs haven't been deprecated:
  doing so without warnings depends on apple#221.

Result:

- We can transform `HTTP2Frame.FramePayload` types to the relevant HTTP
  client and server request and response types.
glbrntt added a commit to glbrntt/swift-nio-http2 that referenced this pull request Jul 30, 2020
@glbrntt
Add HTTP2Frame.FramePayload codecs
5373591 
Motivation:

As part of the work for apple#214 we need new codecs which transform
`HTTP2Frame.FramePayload` to and from the appropriate request and
response parts.

Modifications:

- Add `HTTP2FramePayloadToHTTP1ClientCodec`
- Add `HTTP2FramePayloadToHTTP1ServerCodec`
- Duplicate the HTTP2ToHTTP1CodecTests and update the relevant parts to
  use payloads instead of frames.
- Add relevant test helpers.
- Note: the HTTP2 to HTTP1 (frame based) codecs haven't been deprecated:
  doing so without warnings depends on apple#221.

Result:

- We can transform `HTTP2Frame.FramePayload` types to the relevant HTTP
  client and server request and response types.
@glbrntt glbrntt mentioned this pull request Jul 30, 2020
Merged
glbrntt added a commit to glbrntt/swift-nio-http2 that referenced this pull request Jul 31, 2020
@glbrntt
Add HTTP2Frame.FramePayload codecs
b8d75a4 
Motivation:

As part of the work for apple#214 we need new codecs which transform
`HTTP2Frame.FramePayload` to and from the appropriate request and
response parts.

Modifications:

- Add `HTTP2FramePayloadToHTTP1ClientCodec`
- Add `HTTP2FramePayloadToHTTP1ServerCodec`
- Duplicate the HTTP2ToHTTP1CodecTests and update the relevant parts to
  use payloads instead of frames.
- Add relevant test helpers.
- Note: the HTTP2 to HTTP1 (frame based) codecs haven't been deprecated:
  doing so without warnings depends on apple#221.

Result:

- We can transform `HTTP2Frame.FramePayload` types to the relevant HTTP
  client and server request and response types.
glbrntt added a commit that referenced this pull request Jul 31, 2020
@glbrntt
Add HTTP2Frame.FramePayload codecs (#222)
3c668f5 
Motivation:

As part of the work for #214 we need new codecs which transform
`HTTP2Frame.FramePayload` to and from the appropriate request and
response parts.

Modifications:

- Add `HTTP2FramePayloadToHTTP1ClientCodec`
- Add `HTTP2FramePayloadToHTTP1ServerCodec`
- Duplicate the HTTP2ToHTTP1CodecTests and update the relevant parts to
  use payloads instead of frames.
- Add relevant test helpers.
- Note: the HTTP2 to HTTP1 (frame based) codecs haven't been deprecated:
  doing so without warnings depends on #221.

Result:

- We can transform `HTTP2Frame.FramePayload` types to the relevant HTTP
  client and server request and response types.
Lukasa
Lukasa approved these changes Aug 3, 2020
View reviewed changes
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.

Nice, LGTM.

@glbrntt glbrntt merged commit 98a86c5 into apple:master Aug 3, 2020
@glbrntt glbrntt deleted the gb-create-stream-channel branch August 3, 2020 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@glbrntt @Lukasa