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

PRIORITY_UPDATE can be used for initial priority, split H3 frames into two types #1167

Merged
merged 26 commits into from
Sep 9, 2020

Conversation

LPardue
Copy link
Contributor

@LPardue LPardue commented Apr 30, 2020

This attempts to address #1096 and #1079. It might help towards overcoming the headers vs frames debate.

I made the call to restrict the PRIORITY_UPDATE frame to only be sent by clients.

It replaces ASCII art with new QUIC-style frame definitions.

@LPardue LPardue requested a review from kazuho April 30, 2020 13:45
@LPardue LPardue mentioned this pull request Apr 30, 2020
@LPardue LPardue changed the title Fill out TODOs in PRIORITY_UPDATE, split H3 frames into two types PRIORITY_UPDATE can be used for initial priority, split H3 frames into two types May 4, 2020
Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

It seems like there may be some opportunities for de-duplicating some text, but I don't have a specific suggestion.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
@LPardue
Copy link
Contributor Author

LPardue commented May 7, 2020

Wouldn't it be easier to just define h2's stream zero as the control stream for h2 (at top of document)?

In my head that's what I call it. However, @kazuho has been quite particular with correcting me back use stream zero terminology in the past.

@kazuho WDYT about Roy's suggestion to define stream zero as control in this document?

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
~~~
{: #fig-h2-reprioritization-frame title="HTTP/2 PRIORITY_UPDATE Frame Payload"}
{: #fig-h2-reprioritization-frame title="HTTP/2 PRIORITY_UPDATE Frame"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing the definition of the frame headers for H2 was surprising to me, though I understand that it reflects the change to the HTTP/3 draft. Maybe I should ask if the change in HTTP/3 is a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a personal opinion I prefer to see the frame in whole because it avoids having to cross reference to the frame headers, which are defined in some other document.

But I can drop them if people want. I see that as an editorial change so perhaps we can avoid blocking this and fix up later if needs be.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is an editorial issue, and that we need to ask the WG to resolve (because it is about the convention covering all H2 extensions, not just this one).

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, ALTSVC is the only extension to have reached RFC status previously. It does not include the frame header; only the payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You forgot ORIGIN frame https://tools.ietf.org/html/rfc8336#section-2.1 which also specifies the frame payload.

Now that H2 and H3 have different frame format definitions, I think extensions applicable to both will hit this problem. I'd like to figure what people think is the most acceptable way to present things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One option would be to write documents that update RFC 7838 and RFC 8336 to use the new format and define the H3 frame at the same time...

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I try to block that one out.

They have different frame format definitions, sure; the question is whether the payload needs to vary between H2/H3. I think the real things you need to specify are:

  • What stream it's on (part of the header in H2, intrinsic property in H3)
    • Control stream vs. stream zero language plays into this
  • Flags if used in H2; since H3 doesn't have flags, this might lead to a payload difference
  • Payload

The simplest path for dual-version extensions is not to use flags and only define the payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking the feedback received here and elsewhere, I'm reverted the diagram back to our traditional format.

@kazuho
Copy link
Contributor

kazuho commented May 14, 2020

@LPardue

WDYT about Roy's suggestion to define stream zero as control in this document?

I'd be fine with that, assuming that we'd have the definition somewhere (as @royfielding suggests).

@LPardue
Copy link
Contributor Author

LPardue commented May 15, 2020

This PR changes the HTTP/3 frame format in a breaking way, which is unfortunate because we don't have strong versioning in the extension. It's feasible that interop between endpoints that expect the frame could suffer because they parse them differently.

One idea is to link the priorities draft version to the HTTP/3 draft version. For example, agree that priorities-01 would be used with h3-28. However, the drafts will progress at different rates and we might get stuck.

@kazuho made a suggestion that would avoid this problem. We we pick a large frame type ID that we change for each version of the priorities draft. The final draft that is ready for RFC can then switch back the "nice" lower values. I'll make a commit that actions this proposal in order to get review feedback.

@LPardue
Copy link
Contributor Author

LPardue commented May 15, 2020

127f149 adds "big numbers" for the H3 frame types but leaves the H2 frame as 0xF, mainly because H2's space is only 8 bit and I don't want to pollute it. Maybe others also want an experimental value there but IMO we can progress without it.

@ianswett
Copy link
Contributor

FWIW, I'm ok with changing the frame type value, but in our code it's just as easy(or easier) to switch by draft version.

@LPardue
Copy link
Contributor Author

LPardue commented May 15, 2020

FWIW, I'm ok with changing the frame type value, but in our code it's just as easy(or easier) to switch by draft version.

My concern is that we'll see an uncoordinated staggered rollout of new HTTP/3 endpoints, and that clients talk something different than the server expects, causing the frame parsing to fail and generating a connection error.

@ianswett
Copy link
Contributor

Our servers will likely support draft 25, 27 and 28 at once.

Is the concern that servers will support multiple drafts at once, and clients will typically only support a single version, but server connections won't be aware of what version they're speaking?

I suspect this is a case of different implementations having very different ways of supporting multiple versions of QUIC at once?

@LPardue
Copy link
Contributor Author

LPardue commented May 15, 2020

I hope implementation know what draft they are speaking! I'm more worried about needed to rev the priority draft version multiple times; if we couple to draft 28 we get stuck unless there is a draft 29. I anticipate priorities could iterate more quickly than the specification and more quickly than people might be able to deploy different versions.

Using a unique big number for frame type would even allow implementations to retrofit it to draft 27, which might be more feasible to implement and test in the short term.

@ianswett
Copy link
Contributor

Thanks for clarifying @LPardue, that makes sense. I was a bit confused about how one might not know what version they're speaking.

Copy link
Contributor

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. Looks good modulo one point below.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
Unlike the Priority header field, the PRIORITY_UPDATE frame is a hop-by-hop
signal. Clients that do not want to expose an end-to-end signal MAY omit the
header field but send the PRIORITY_UPDATE frame at an early moment assuming that
it will reach the server as early as the request message it applies to. The
Copy link
Contributor

Choose a reason for hiding this comment

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

The important "when" here is "before prioritization information is not useful to the server". That might be later than when the server receives the request.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
Comment on lines 442 to 443
references has been created. The Stream ID MUST be within the client-initiated
bidirectional stream limit. If a server receives a PRIORITY_UPDATE for a Stream
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be qualified; if the frame contains a Push ID, then this doesn't apply.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
LPardue and others added 10 commits May 22, 2020 15:01
thanks!

Co-authored-by: ianswett <[email protected]>
Strawman proposal.

In HTTP/3 we have a huge space of possible frame types, so lets use
them during draft development. The values in this commit were picked
by generating a set of GREASE values, selecting one and adding 1
and 2 to it.

The HTTP/2 frame type remains as 0xF because it is only 8-bits,
meaning there is less room to play and greater chance of polluting a
small range. However, I also expect less churn in H2, partly because
I anticipate that implementers of the draft will focus on HTTP/3.

Since we intend to reserve the values 0xF and 0x10 in the final RFC,
I've left the IANA consideration unchanged. I have reserved the
selected experimental values in the temporary IANA registry at
https://github.com/quicwg/base-drafts/wiki/Temporary-IANA-Registry.
@LPardue LPardue force-pushed the priority-update-frames branch from 11f28fd to 41432cb Compare May 22, 2020 14:04
@LPardue
Copy link
Contributor Author

LPardue commented May 26, 2020

We also discussed this PR during the May 2020 virtual interim and the conversation segued into a discussion about reprioritization which is related but ultimately is tracked separately in #1021.

As editor, I believe this PR adds changes that better describe the protocol with the design it had when adopted by the WG. And therefore it can be merged independent of the WG finding consensus on reprioritization.

I've received a lot of feedback on this PR (thanks) and believe I've addressed all but one comment. I aim to resolve that one soon and then plan to merge this PR unless I hear some pushback.

The HTTP/3 PRIORITY_UPDATE frame (type=0xF) carries the identifier of the
element that is being reprioritized, and the updated priority in ASCII text,
using the same representation as that of the Priority header field value.
The HTTP/3 PRIORITY_UPDATE frame (type=0x1CCB8BBF1F0700 or 0x1CCB8BBF1F0701) is
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it 0x1CCB8BBF1F0700/0x1CCB8BBF1F0701 or 0x10/0x11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In HTTP/3, the value should be the long 0x1CCB8BBF1F0700/0x1CCB8BBF1F0701, I'll fix this up.

@LPardue LPardue force-pushed the priority-update-frames branch from 8b064e5 to ae7ef87 Compare September 7, 2020 22:18
@LPardue
Copy link
Contributor Author

LPardue commented Sep 7, 2020

This issue got tied up with the discussion about reprioritization. Now that the chairs believe there is rough consensus on the matter, I'm giving fair warning that I'm aiming to land this change in the next day or two. I'll remind folks that this has had pretty substantial review already and I'm not looking for relitigation.

I've just pushed two commits: 1) revert the H2 frame diagram format, we can tackle this later if we care. 2) fix an inconsistency in frame type values report by @guoye-zhang.

There is an open question about a servers ability to understand a safe limit of buffering, which might be subject to implementation API. I'd like to spin that off into it's issue so we can make progress here.

@LPardue
Copy link
Contributor Author

LPardue commented Sep 8, 2020

FYI I had some feedback that the frame type values were a bit cumbersome so I've shortened them to 0xF0700 and 0xF0701. See b007e93

Copy link
Contributor

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

@LPardue Thank you for all the work. LGTM, just editorial suggestions.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

All of these are nits except for the "within the stream limit" conversation on the HTTP/2 piece. Feel free to split that to a separate issue if needed.

draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-priority.md Outdated Show resolved Hide resolved
@LPardue LPardue merged commit 8eb70f8 into master Sep 9, 2020
@LPardue LPardue deleted the priority-update-frames branch September 9, 2020 21:13
richanna pushed a commit to richanna/http-extensions that referenced this pull request Oct 20, 2020
…o two types (httpwg#1167)

* Fill out TODOs in PRIORITY_UPDATE, split H3 frames into two types

* Allow PRIORITY_UPDATE as initial signal

* Reserved the selected experimental frame type values in the temporary IANA registry at
https://github.com/quicwg/base-drafts/wiki/Temporary-IANA-Registry.

* Incorporate a bunch of suggestions from Roy and others

Co-authored-by: ianswett <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
Co-authored-by: Kazuho Oku <[email protected]>
Co-authored-by: Mike Bishop <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants