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

DUPLICATE_PUSH instead of multiple PUSH_PROMISE frames #2072

Merged
merged 4 commits into from
Dec 4, 2018

Conversation

MikeBishop
Copy link
Contributor

@MikeBishop MikeBishop commented Nov 29, 2018

Fixes #1864, by only sending the request headers once and having a separate frame to pick them up by reference. Also would have fixed #1959 along the way, as an added bonus. (Separated into new PR so I can fix some other PRIORITY things.)

However, this is not an unmitigated improvement. The point of the ordering requirement for PUSH_PROMISE frames is that the client knows that a push is coming before they see references to a dependent resource and initiate their own requests. If reordering (or blocking QPACK references) places a DUPLICATE_PUSH ahead of the corresponding PUSH_PROMISE, knowing that the resource is on the way can no longer be guaranteed.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Nov 29, 2018
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I think that this is a net gain. It's going to be annoying to implement, but hopefully not too annoying.

draft-ietf-quic-http.md Show resolved Hide resolved
When a client request is first sent or a placeholder first allocated, the
element is dependent on the root of the priority tree. Pushed streams are
initially dependent on the client request on which the PUSH_PROMISE frame was
sent. In all cases, elements are assigned an initial weight of 16.
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice side-effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to #2074 so I could do more priority-related changes without blocking on this, but I agree. :-)

draft-ietf-quic-http.md Outdated Show resolved Hide resolved
Copy link
Member

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

Lgtm, the saving in bandwidth and client processing are clear wins.

@MikeBishop MikeBishop merged commit f0ac9e4 into master Dec 4, 2018
@MikeBishop MikeBishop deleted the http/duplicate_push branch February 6, 2019 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
4 participants