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

Allow depends_on and weight settings in send_headers #229

Closed
Kriechi opened this issue May 21, 2016 · 4 comments
Closed

Allow depends_on and weight settings in send_headers #229

Kriechi opened this issue May 21, 2016 · 4 comments

Comments

@Kriechi
Copy link
Member

Kriechi commented May 21, 2016

Currently the send_headers function on the connection (and stream) object does not allow us to specify a depends_on and weight setting. It would be useful to create a single HeadersFrame with these values, instead of creating an additional PriorityFrame.

A related problem is that a RequestReceived event does not carry these values - but instead a second event (PriorityUpdated) is issued - which makes it unnecessary difficult to reassemble the original HeadersFrame. This might also be related to #228 for handling multiple events of the same stream etc.

Right now, once I receive a RequestReceived event, I'm looping through all remaining events to find possible PriorityUpdated which I would like then to merge all values into a new send_headers call.

Let me know if this is a feasible request - or maybe I'm just missing a nifty API feature.
Thanks.

edit
How can change the priority of a stream? I couldn't find a way to generate a PriorityFrame - or do I have to do that manually with raw frames?

@Lukasa
Copy link
Member

Lukasa commented May 21, 2016

So this is two separate concerns. Let's make them real explicit. =)

  1. Emitting priority information. Yes, the API currently has no support for it, and we should definitely add it. We'll want to add two things: extend the send_headers callable and add a new prioritize (or similar) callable.
  2. PriorityUpdated and *Received events. In general I'm happy to keep these as separate events, but I'm acutely aware that it makes it very difficult to reproduce the original frame exactly. I'd like to get an idea of how important that is for mitmproxy, because right now hyper-h2 already doesn't guarantee that that is possible (e.g. we hide CONTINUATION frames from the user). If it's important, we can consider keeping hold of the frame object and attaching it to the event itself, which might make this easier.

@Kriechi
Copy link
Member Author

Kriechi commented May 22, 2016

Ideally we (@mitmproxy) would like to reproduce frames as accurate as possible.

I'm not sure if there is a semantic difference between HEADERS with priority information, and HEADERS+PRIORITY frames. But I would rather reflect a single incoming HEADERS frame with the same frame sending out - and not having to generate two frames...
Adding code to implement concern 1 (as per your list) would be sufficient for as at the moment.

Nevertheless, it would be nice to distinguish between priority information from a HEADERS frame (we would call send_headers with additional arguments), and an independently received PRIORITY frame (we would need to call h2_conn.prioritize).

For CONTINUATION frames this might be a bit tricky, but since we already mirror MAX_FRAME_SIZE from client to server, the header_block_fragment splitting should occur at the same length - producing the same amount of frames - at least that's what we are hoping for.

@Lukasa
Copy link
Member

Lukasa commented May 23, 2016

For CONTINUATION frames this might be a bit tricky, but since we already mirror MAX_FRAME_SIZE from client to server, the header_block_fragment splitting should occur at the same length - producing the same amount of frames - at least that's what we are hoping for.

I don't think that happens: I've definitely seen Chrome emit CONTINUATION frames when emitting header blocks well below the MAX_FRAME_SIZE that Chrome sets. I'd have to go back and double-check, but I'm pretty confident Chrome does weird stuff here.

Regardless, my reading is that part 1 of this is important to you and we should shoot to do it soon, but that part 2 is worth taking longer to think over.

While we're here, you may want to look at @rbtcollins' suggestions in #228. This includes the idea of showing "simultaneous" events by emitting a frozenset containing both of them at once. If we adopted that API change you actually would be able to tell the difference between HEADERS then PRIORITY and HEADERS with PRIORITY extension, because the first would emit two separate events and the second would emit a frozenset with both. How does that idea sound?

@Kriechi
Copy link
Member Author

Kriechi commented May 25, 2016

Sounds all good! 👍

If we continue the path to "simultaneous events" (or something similar), we could add the information in how many chunks the headers were split ot RequestReceived and ResponseReceived, e.g. we could add a list with the header_block_fragment sized, which translates into how many CONTINUATION frames have been received.

Of the top of my head I think events can simply be grouped by their associated stream-id.

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

No branches or pull requests

2 participants