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

Prepare forwards-compatibile Body trait #2840

Closed
2 tasks done
seanmonstar opened this issue May 20, 2022 · 10 comments
Closed
2 tasks done

Prepare forwards-compatibile Body trait #2840

seanmonstar opened this issue May 20, 2022 · 10 comments
Assignees
Labels
A-body Area: body streaming. C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Milestone

Comments

@seanmonstar
Copy link
Member

seanmonstar commented May 20, 2022

We want to explore a trait that can be forwards-compatible in frame types for a body stream.

Steps:

  • Accept a proposal
  • Implement in the http-body crate.
@seanmonstar seanmonstar added C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. B-rfc Blocked: More comments would be useful in determine next steps. A-body Area: body streaming. labels May 20, 2022
@seanmonstar seanmonstar added this to the 1.0 milestone May 20, 2022
@seanmonstar
Copy link
Member Author

Proposal

Problem

The Body trait provides methods to allow basically 2 frame types to be passed along: data, and trailers. There's a video overview, but I'll summarize: in order to be Flexible, we want to allow Bodys to transmit frame types that hyper doesn't even know about. We can't do that with per-frame-type methods.

Frames

We want a Frame like thing that users can poll (and send). It would be convenient if it behaved like an enum, where they could match on the variants, but there's a forwards-compatibility trap. Consider:

match f {
    Frame::Data(buf) => {},
    Frame::Trailers(map) => {},
    Frame::Unknown(raw) => {},
    // #[non_exhaustive]
    _ => {},
}

If we were to start to "understand" a frame type that used to fall into the Unknown variant, and created Frame::Priority(pri), then user code that updated would suddenly no longer be handling the frame.

So, we don't want to expose an enum directly. Something like the dyn match concept would be great, but it doesn't exist yet.

It'd be possible to inspect the frame using a bunch ifs. But Rust programmers like the feel of match, so a macro could do it.

Proposed Design

pub trait Body {
    type Data: Buf;
    fn poll_frame(..) -> Result<Option<Frame<Self::Data>>>;
}

pub struct Frame<T>(Kind<T>);

enum Kind<T> {
    // The first two variants are "inlined" since they are undoubtedly
    // the most common. This saves us from having to allocate a
    // boxed trait object for them.
    Data(T),
    Trailers(HeaderMap),
    Unknown(Box<dyn Frameish>),
}

/// The name is just YOLO for now.
pub trait Frameish {
    fn frame_kind(&self) -> u64;
    // Just a byte slice? The DATA type couldn't necessarily fulfill this...
    fn payload(&self) -> &[u8];
}

Usage

Matching

while let Some(fr) = body.frame().await? {
    // the macro is optional, but seems helpful
    match_dyn! { fr;
        frame::Data(buf) => {
            
        },
        frame::Trailers(trailers) => {
            
        },
        other => {
            if other.frame_kind() == 0xF3 {
                eprintln!("my custom frame: {:?}", other.payload());
            } else {
                // i dunno?
            }
        }
        
    }
}

Buffered

The Buffered util type provides methods to await specific frame types. It can do this by doing the dyn-match internally, and if it doesn't match the expected type, put the frame back into a slot to poll for next time.

let mut buffered = body.buffered();

// Buffered can poll for specific frame types
while let Some(data) = buffered.data().await? {
    
}

// DATA frames are done. What stopped them? A trailers frame? EOS?
if let Some(trailers) = buffered.trailers().await? {
    
}
// or maybe some other EOS-ish frame
if let Some(fr) = buffered.frame().await? {
    // ``\_(0_0)_/``
}

Sending

Assume a channel where the receiver is impl Body.

let (tx, rx) = hyper_util::body::channel();
// give away the rx

tx.send_frame(Frame::data(buf)).await?;
tx.send_frame(Frame::trailers(map)).await?;
tx.send_frame(Frame::custom(0xF3, b"yolo")).await?;

Remaining Questions

How does dyn_match! convert from to frame::Data?

Just a quich sketch:

impl<T> Frame<T> {
    pub fn is_data(&self) -> bool;
    pub fn into_data(self) -> Option<frame::Data<T>>;
    
    pub fn is_trailers(&self) -> bool;
    pub fn into_trailers(self) -> Option<frame::Trailers>;
    
    pub fn frame_kind(&self) -> u64;
}

Do we seal the Frameish trait?

Should users implement the type for their own new frame kinds?

Or should they just use Frame::custom(id, payload)?

@seanmonstar
Copy link
Member Author

cc @acfoltzer @davidpdrsn @nox @olix0r

You all either maintain proxies that could need this, or previously done a lot with http-body trait.

@seanmonstar
Copy link
Member Author

Also @LucioFranco and @rcoh.

@seanmonstar
Copy link
Member Author

I'm going to move forward with this, unless anyone wants to comment.

@LucioFranco
Copy link
Member

Oh sorry missed this, What would exist in http-body vs what would exist in hyper? Would all the frame stuff go into the body crate? I think this makes sense though I am curious to see how it works in practice.

@seanmonstar
Copy link
Member Author

Yes pretty much everything proposed here is actually for http-body.

@HyeonuPark
Copy link

HTTP 2 and 3 have slightly different meanings on their frame type ids. The body may need to provide information about its underlying connection protocol.

@seanmonstar
Copy link
Member Author

That's a good point. HTTP/3 tried to reuse the same IDs for equivalent frames, but there may be subtle differences, and it could be worth including a version() method to a frame. Or, perhaps it should be up to the user to set the version() on the corresponding Request/Response`. Either way, doesn't need to be determined immediately.


I'm moving forward with the proposal.

@seanmonstar seanmonstar removed the B-rfc Blocked: More comments would be useful in determine next steps. label Sep 8, 2022
@seanmonstar seanmonstar changed the title Explore forwards-compatibile Body trait Prepare forwards-compatibile Body trait Sep 8, 2022
seanmonstar added a commit to hyperium/http-body that referenced this issue Sep 8, 2022
This implements the core of the proposal in
hyperium/hyper#2840.
@jhatala
Copy link

jhatala commented Sep 9, 2022

Hello, @seanmonstar! @acfoltzer and I work for the same company. This is great progress and we are looking forward to seeing how custom HTTP/2 frames will be handled in hyper. At the moment we have a patched h2 crate that has special handling for our custom frame, which works fine, but it is always nice to see standard interfaces for things.

A few things spring to mind related to the plan:

In addition to frame_kind and payload, I think that it would be useful for Frameish to have an accessor for flags. In our internal HTTP/2-based protocol we use the flags field in the frame header of our custom frames for distinguishing between different payload formats.

What do you think would be the best way to represent custom frames exchanged before the headers frame? In our internal HTTP/2-based protocol a custom frame with a stream id can precede the headers frame. In that case, rather than the headers frame creating the new stream, it is the custom frame that creates it. We use the extensions/extensions_mut interface on the http crate's Request and Response as a super convenient way to retrieve the custom frame(s) from the request and to attach it to the response.

We also use custom settings, though this is getting further and further away from the topic of the Body trait.

@seanmonstar
Copy link
Member Author

it would be useful for Frameish to have an accessor for flags

Hm, yea, flags would be good to support too. Would you like to push forward on exploring the design for custom frames? I've got a deadline for a hyper 1.0 release candidate soon, and so my main priority is getting the types in place, such as Body::poll_frame and an extensible Frame, and then allow the custom parts to be added once ready.

What do you think would be the best way to represent custom frames exchanged before the headers frame?

The design doesn't accommodate that at all, since I've been assuming what it says in the spec is that it's illegal:

Receiving any frame other than HEADERS or PRIORITY on a stream in this state MUST be treated as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

I... think at that point I'd probably customize at the codec level, like the h2 crate. (I'd love to support what you're doing generally, as long as it keeps our goal of making hyper a spec-compliant HTTP library.)

seanmonstar added a commit to hyperium/http-body that referenced this issue Oct 11, 2022
This implements the core of the proposal in
hyperium/hyper#2840.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
seanmonstar added a commit that referenced this issue Oct 24, 2022
The `Body` trait was adjusted to be forwards compatible with adding new
frame types. That resulted in changing from `poll_data` and `poll_trailers`
to a single `poll_frame` function. More can be learned from the proposal
in #2840.

Closes #3010

BREAKING CHANGE: The polling functions of the `Body` trait have been
  redesigned.

  The free functions `hyper::body::to_bytes` and `aggregate` have been
  removed. Similar functionality is on
  `http_body_util::BodyExt::collect`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-body Area: body streaming. C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants