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

New HTTP/2 currency types. #44

Merged
merged 1 commit into from
Feb 26, 2019
Merged

New HTTP/2 currency types. #44

merged 1 commit into from
Feb 26, 2019

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Feb 25, 2019

Motivation:

As discussed in our HTTP/2 types manifesto, it is helpful to run more
metadata through the HTTP/2 pipeline. This makes it easier to write
ChannelHandlers that fully participate in the HTTP/2 state machine
while avoiding needing to too tightly couple the handlers.

Modifications:

  • Added new HTTP/2 pipeline data types.
  • Removed HTTP2Frame.FrameFlags, moved flags into relevant payloads.

Result:

Right now, not much. All tests pass with new payload types. The full
data path rewrite will follow in a separate patch, as this one is large
enough.

@Lukasa Lukasa requested a review from weissi February 25, 2019 14:11
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

this looks really good! A few nits

/// A single HTTP/2 frame, along with the state change it triggered in the state machine.
public struct HTTP2FrameWithMetadata {
/// The received HTTP/2 frame.
var frame: HTTP2Frame
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't those vars be public

/// the state change triggered by parsing this frame.
public struct HTTP2FrameWithMetadataRequest {
/// The HTTP/2 frame to send.
var frame: HTTP2Frame
Copy link
Member

Choose a reason for hiding this comment

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

and these guys too?

Sources/NIOHTTP2/HTTP2Frame.swift Outdated Show resolved Hide resolved
@Lukasa Lukasa force-pushed the cb-new-data-flow branch 2 times, most recently from 0b7245e to e2f085a Compare February 25, 2019 15:30
Motivation:

As discussed in our HTTP/2 types manifesto, it is helpful to run more
metadata through the HTTP/2 pipeline. This makes it easier to write
ChannelHandlers that fully participate in the HTTP/2 state machine
while avoiding needing to too tightly couple the handlers.

Modifications:

- Added new HTTP/2 pipeline data types.
- Removed HTTP2Frame.FrameFlags, moved flags into relevant payloads.

Result:

Right now, not much. All tests pass with new payload types. The full
data path rewrite will follow in a separate patch, as this one is large
enough.
@Lukasa
Copy link
Contributor Author

Lukasa commented Feb 26, 2019

Ok, ready for re-review.

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

🚢it, that looks good!

@Lukasa Lukasa merged commit 24f67d8 into apple:master Feb 26, 2019
@Lukasa Lukasa deleted the cb-new-data-flow branch February 26, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants