Define initial PRIORITY frame and remove exclusive dependencies #2075

MikeBishop · 2018-11-29T23:59:33Z

Fixes #1865. The observations from the issue discussion are:

HTTP/2's "exclusive" prioritization (which make the prioritized element the sole child of the parent, making all current parents children of the prioritized element instead) is the cause of many ordering headaches and actually makes it impossible to guarantee an eventually-consistent view of priority between client and server. Fundamentally, it changes the priority of elements which aren't mentioned in the frame, and that's annoying.
If a PRIORITY frame gets delayed and a request is quickly processed, it may be processed immediately (default is pretty high) and only later be discovered to be unimportant.

In the absence of exclusive dependencies, it's possible for a request stream to carry its own initial priority, as @dtikhonov proposed. This is guaranteed to be the first time that a priority is assigned to this request, and subsequent PRIORITY frames always override it. (If a PRIORITY frame gets reordered ahead, this initial PRIORITY is ignored.)

This is a further departure from RFC 7540's priority scheme, but as it appears exclusive priorities and reordering are somewhat incompatible, I believe is still within our design space.

MikeBishop · 2018-11-30T00:00:45Z

(I considered making this a stream header on request streams, but since it's optional, decided a frame type was probably more appropriate. I'm open to being convinced otherwise.)

martinthomson

I don't know if the exclusive thing need to be bundled in here.

There needs to be more text in this about managing races with PRIORITY frames. You have resolved the question of which one takes precedence, but you should mention that the identified dependency might have default priority when this is received, resulting in potentially inconsistent prioritization. Not a big problem, but worth noting I think.

@ddragana, do you know if we use the exclusive thing in our priority setup at all?

martinthomson · 2018-11-30T01:06:39Z

draft-ietf-quic-http.md

@@ -496,13 +496,9 @@ The PRIORITY frame payload has the following fields:
  : A two-bit field indicating the type of element being depended on.

  Empty:
-  : A three-bit field which MUST be zero when sent and MUST be ignored
+  : A four-bit field which MUST be zero when sent and MUST be ignored
    on receipt.


Not really for this, but do we have to mandate ignoring reserved fields?

If you don't, you risk an overzealous implementation enforcing the "MUST be zero" and we can't use it later without negotiation. "MUST ignore" seems cleaner than "MUST not die in flames if it's not zero."

martinthomson · 2018-11-30T01:07:51Z

draft-ietf-quic-http.md

+The INITIAL_PRIORITY frame payload has the following fields:
+
+  Dependency Type:
+  : A two-bit field indicating the type of element being depended on.


The alignment of the dependency type in comparison to PRIORITY is unfortunate.

True -- I considered having a separate two-bit empty field, but that also felt unfortunate. If you think that's the better of two evils, I'm fine.

Considering the fact that we are changing the definition of the PRIORITY frame, it might make sense to change the order of the flags and fields as well.

Specifically, we can move the PT to the end of the flags and PEID to the end of the fields. That would help us having a unified decoder for the two frames. Additionally, we might call PEID an optional field that only exists when the frame is sent over a control stream, thereby unifiying the frame definition to one.

One alternative would to have a static two-bit field that is always set to the value of 11. There's no extra overhead since there's unused reserved bits.

However, I like @kazuho's proposal and think there is room to extend it further.

The PRIORITY frame definition says that

Element Dependency ID: A variable-length integer that identifies the element on which a dependency is being expressed. Depending on the value of Dependency Type, this contains the Stream ID of a request stream, the Push ID of a promised resource, the Placeholder ID of a placeholder, or is ignored.

IIUC the ignored case properly, wouldn't it be more efficient to make the EDID (chuckle, not this one) optional too, based on the prioritisation type. I.e. if the dependency is root of the tree, don't send an ID.

That actually could combine nicely -- rather than 11 meaning "the root of the tree," it means "the current stream," and we keep the restriction that reprioritizing the root of the tree is illegal. So 11 means this stream when it's send on a request, but means the root when it's sent on the control stream.

I was trying not to have conditionally-present fields, but if folks are okay with that, I could try that variant.

Using 11 to identify the current stream (and therefore the omission of the field) makes sense.

I was trying not to have conditionally-present fields, but if folks are okay with that, I could try that variant.

FWIW, the reason I am happy with having conditionally-present fields is that it saves bytes: omitting PEID in the INITIAL_PRIORITY frame saves one byte per request in the common, omitting EDID in the PRIORITY frame saves at least one byte in the first flight when placeholders are used.

I think omitting having bytes with no meaning is more consistent to the efforts we have spent in other parts of the transport and the h3 drafts to minimize the overhead.

MikeBishop · 2018-11-30T07:02:33Z

The necessity of the Exclusive change might be debatable, but I think it is needed. Without Exclusive, there's an obvious race -- INITIAL_PRIORITY is racing with an Exclusive dependency on the parent, and the outcome on the server is indeterminate. With the exclusive change, it's possible to make the outcome of this addition deterministic.

RyanTheOptimist · 2018-12-01T00:19:55Z

With HTTP/2, the initial HEADERS frame carries the priority information for the request. I wonder if instead of introducing an INITIAL_PRIORITY frame it would be simpler to bundle this initial priority information into the HEADERS frame instead. That would have the advantage of matching HTTP/2.

kazuho · 2018-12-01T01:17:41Z

@RyanatGoogle

I wonder if instead of introducing an INITIAL_PRIORITY frame it would be simpler to bundle this initial priority information into the HEADERS frame instead. That would have the advantage of matching HTTP/2.

I am afraid that creates a race condition. If a PRIORITY frame of stream X says that it depends on stream Y, and if PRIORITY frame of stream Y says it depends on X, the server cannot tell which to apply first. IIUC, the observation behind this PR is that the ordering between INITIAL_PRIORITY and PRIORITY frames can be recovered; i.e., any reference to a stream from a PRIORITY frame is guaranteed to occur after the INITIAL_PRIORITY frame of that frame is issued.

LPardue · 2018-12-02T20:45:22Z

draft-ietf-quic-http.md

@@ -825,6 +821,70 @@ MUST be treated as a connection error of type HTTP_MALFORMED_FRAME.
 A server MUST treat a MAX_PUSH_ID frame payload that does not contain a single
 variable-length integer as a connection error of type HTTP_MALFORMED_FRAME.

+### INITIAL_PRIORITY {#frame-initial-priority}
+
+The INITIAL_PRIORITY (type=0x0F) frame specifies the client-advised priority of


To maintain consistency with PRIORITY frame, you could use "sender-advised priority". Language elsewhere qualifies that the only sender that is allowed is a client.

or,

change it to client-advised in PRIORITY

LPardue · 2018-12-02T20:51:54Z

draft-ietf-quic-http.md

+INITIAL_PRIORITY frame sent on a non-request stream MUST be treated as a
+connection error of type HTTP_WRONG_STREAM.  An INITIAL_PRIORITY frame received
+after other frames or received by a client MUST be treated as a stream error of
+type HTTP_UNEXPECTED_FRAME.


Suggestion to break this out

INITIAL_PRIORITY frames MAY be sent as the first frame of a
request stream and MUST NOT be sent thereafter. An INITIAL_PRIORITY
frame received after other frames MUST be treated as a stream error of
type HTTP_UNEXPECTED_FRAME.

INITIAL_PRIORITY frames sent on a non-request stream MUST be treated as a
connection error of type HTTP_WRONG_STREAM.

INITIAL_PRIORITY frames received by a client MUST be treated as a stream error of
type HTTP_UNEXPECTED_FRAME.

Now wondering if it would it be clearer to say:

INITIAL_PRIORITY frames are OPTIONAL. When used, the frame MUST be sent as the first frame of a request stream. An INITIAL_PRIORITY frame received after other frames MUST be treated as a stream error of type HTTP_UNEXPECTED_FRAME.

LPardue · 2018-12-02T21:08:39Z

draft-ietf-quic-http.md

+    is being expressed. Depending on the value of Dependency Type, this contains
+    the Stream ID of a request stream, the Push ID of a promised resource, the
+    Placeholder ID of a placeholder, or is ignored.  For details of
+    dependencies, see {{priority}} and {{!RFC7540}}, Section 5.3.


Ignore me...

LPardue · 2018-12-02T21:10:54Z

draft-ietf-quic-http.md

+
+When an INITIAL_PRIORITY frame claims to reference a request, the associated ID
+MUST identify a client-initiated bidirectional stream.  A server MUST treat
+receipt of PRIORITY frame with a Stream ID of any other type as a connection


s/PRIORITY frame/INITIAL_PRIORITY frame

LPardue

I think the general theme of the PR is good. I've made some minor suggestions.

The big question is: new frame or rework existing PRIORITY frame. I'll see how that conversation develops.

MikeBishop · 2018-12-03T20:16:52Z

I considered making it part of either HEADERS or simply a stream header like the Push ID, since it MUST always come first. My reasoning was that, since it's not always going to be used, a stream header would be overkill. The conditional presence in HEADERS in H2 was based on flags, which we don't have any more 😳, so that seemed to present its own problems, plus the restriction that it only appear on the first HEADERS of a stream. It seemed simpler to restrict those semantics to their own frame type.

kazuho · 2018-12-03T21:29:23Z

+1 to having PRIORITY as a separate frame on the request stream. IMO, we should refrain from wasting bytes of a HEADERS frame, because it is used for carrying the responses as well. Bandwidth from the server to the client is the most precious resource in HTTP.

kazuho

Looks like a nice improvement to me.

In addition to guaranteeing that the server would have the prioritization information, the proposed change saves multiple bytes (in addition to the points I clarified in #2075 (comment), we also save at least 3 bytes per packet by removing the need to emit a STREAM frame of a control stream when sending requests).

LPardue · 2018-12-03T22:27:02Z

Lgtm

MikeBishop · 2018-12-04T18:56:46Z

It's worth noting one of @kazuho's earlier comments raises a race condition that still appears possible here:

Request A or Control: PRIORITY( A -> B )
Request B: PRIORITY( B -> A )
Control: PRIORITY( A -> C )

These two initial frames race on parallel streams. Which is the child of the other? When A becomes a child of C, does B come with it?

dtikhonov · 2018-12-04T19:08:29Z

What if we duplicate the priority information on the control stream? If the priority of the new stream has already been set by the control stream, ignore the priority information on the request stream.

MikeBishop · 2018-12-04T19:31:08Z

Yes, I believe that ensures there's a complete ordering and eventual consistency. Even if an initial PRIORITY frame on a different stream gets things off-kilter briefly, the control stream will ensure the correct order wins in the end. With a quick exploration of the sequence above, I can't find one in which the end state is incorrect.

Should that be "MAY/SHOULD repeat the desired prioritization on the control stream if dependencies are modified in close succession," or do you think repeating it should be a hard requirement?

dtikhonov · 2018-12-04T20:33:45Z

I think a hard requirement is too expensive. The only reason to duplicate the priority information from the request stream is if the priority changes, otherwise it's a waste.

Case 1: superfluous duplication

Request A: PRIORITY( A -> B )
Control: PRIORITY( A -> B )

Case 2: race possible

Request A: PRIORITY( A -> B )
Control: PRIORITY( A -> C )

Case 3: ensure eventual consistency

Request A: PRIORITY( A -> B )
Control: PRIORITY(A - B); PRIORITY( A -> C )

My guess is that most clients will fall under Case 1. If request priority changes, the client should use Case 3.

kazuho · 2018-12-04T21:10:41Z

@MikeBishop

It's worth noting one of @kazuho's earlier comments raises a race condition that still appears possible here:

Request A or Control: PRIORITY( A -> B )
Request B: PRIORITY( B -> A )
Control: PRIORITY( A -> C )

These two initial frames race on parallel streams. Which is the child of the other? When A becomes a child of C, does B come with it?

FWIW, I do not think we have a race here.

For the case of:

Request A: PRIORITY(A -> B)
Request B: PRIORITY(B -> A)
Control: PRIORITY(A -> C)

I am not sure if that would ever happen.

Having request A's initial PRIORITY specifying B as it parent and request B's initial PRIORITY specifying A means that there was a loop when the client created the two streams.

That's a peculiar behavior of a client and I do not think that we need to address that.

For the case of:

Control: PRIORITY(A -> B)
Request B: PRIORITY(B -> A)
Control: PRIORITY(A -> C)

Under the premise that the client adds requests to the priority tree one by one and at the same time emits a INITIAL_PRIORITY frame, and that further modifications are signaled using the control streams, a server can determine the order of this pattern; PRIORITY(B -> A) predated PRIORITY(A -> B).

MikeBishop · 2018-12-04T21:28:58Z

For your second, there's no loop. B depends on A, regardless of A's ancestry. B might attach to A before or after it gets reparented, but that's fine.

For the first, though, there's defined behavior (in 7540) of creating a dependency on a descendant -- that descendant implicitly moves up to become a peer, then the dependency stated in the priority frame is created. A client trying to leverage this behavior could produce that sequence.

Another option would be to instead say that declaring a dependency on a descendant is always an error, and you need to explicitly perform the elevation-to-peer step (on the control stream). That would be consistent with removing exclusive dependencies, because it's eliminating another implicit mutation of the priority tree.

kazuho · 2018-12-04T21:42:25Z

For the first, though, there's defined behavior (in 7540) of creating a dependency on a descendant -- that descendant implicitly moves up to become a peer, then the dependency stated in the priority frame is created. A client trying to leverage this behavior could produce that sequence.

I think I might have been unclear.

My point is that PRIORITY(A -> B) carried on a request stream implies that stream B existed when A was created, and that PRIORITY(B -> A) implies the reverse.

Because streams are created in order, that would never happen "under the premise that the client adds requests to the priority tree one by one and at the same time emits a INITIAL_PRIORITY frame".

(copied the last subsentence from my comment above that explained the premise for the different case)

MikeBishop · 2018-12-05T00:29:17Z

I see the argument that a well-constructed client will not emit an invalid pattern. 🙂 It's not obvious to me how to turn that into a rule the server would use for interpretation or prohibition of those patterns. Maybe a prohibition on referencing higher stream numbers in the initial PRIORITY frame?

kazuho · 2018-12-05T01:13:05Z

I see the argument that a well-constructed client will not emit an invalid pattern. 🙂 It's not obvious to me how to turn that into a rule the server would use for interpretation or prohibition of those patterns. Maybe a prohibition on referencing higher stream numbers in the initial PRIORITY frame?

👍 I like the idea of banning referring to higher stream numbers in the initial PRIORITY frame.

My argument was simply that we can ignore the case (because nobody would do that), but I agree that it would be even better if we can detect and prohibit such misuse.

MikeBishop · 2018-12-05T18:55:41Z

Updated -- PTAL.

kazuho

@MikeBishop Thank you for the changes. The PR looks fine modulo the concerns stated below.

kazuho · 2018-12-05T22:38:11Z

draft-ietf-quic-http.md

-HTTP_MALFORMED_FRAME.  Likewise, a PRIORITY frame sent on a control stream that
-prioritizes the current stream MUST be treated as a connection error of type
-HTTP_MALFORMED_FRAME.
+prioritizes any other stream or expresses a dependency on a request with a


I am not sure if we need "prioritizes any other stream or".

Now that exclusive flag is going away, is there any case that could hit this condition?

This is a perhaps-unclear reference to a PRIORITY frame where Prioritized Element Type is not 11 (i.e. the thing being prioritized is anything other than "this stream").

Ah. Thank you for the clarification. I missed that.

My preference goes to stating that Prioritized Element Type MUST be 11 when the frame is sent on a request stream and that it MUST NOT be 11 when the frame is sent on a control stream.

This is because I prefer to avoid having multiple representation that means the same; under the current provision, it seems that PEID could be encoded in two ways for a PRIORITY frame sent on a request stream; either set Prioritized Element Type to 11 or set it to 00 and set Prioritized Element ID to the ID of the stream. I fear that we might see interoperability issues in the wild if most of the clients use the 11 form but some used the 00 form.

Good point. The requirement for the values was already there (in the definition of Prioritized Element Type), but I'll scope the required-error language to the specific values as well.

kazuho · 2018-12-05T22:42:45Z

draft-ietf-quic-http.md

+greater Stream ID than the current stream MUST be treated as a stream error of
+type HTTP_MALFORMED_FRAME.  Likewise, a PRIORITY frame sent on a control stream
+that prioritizes the current stream MUST be treated as a connection error of
+type HTTP_MALFORMED_FRAME.


I am not sure what the intent of the last sentence is.

My understanding is that a PRIORITY frame can only refer to a "Stream ID of a request stream" (assuming that the type bits are 00 (i.e. Request stream). Therefore, I think that we do not need this sentence.

This is the inverse of the above -- sending 11 on a control stream, indicating that the thing being prioritized is the control stream itself.

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

MikeBishop requested review from martinthomson, dtikhonov and LPardue November 29, 2018 23:59

martinthomson reviewed Nov 30, 2018

View reviewed changes

dtikhonov approved these changes Nov 30, 2018

View reviewed changes

LPardue reviewed Dec 2, 2018

View reviewed changes

LPardue approved these changes Dec 2, 2018

View reviewed changes

kazuho approved these changes Dec 3, 2018

View reviewed changes

MikeBishop changed the base branch from http/initial_priority to master December 3, 2018 21:43

MikeBishop added 4 commits December 3, 2018 13:46

INITIAL_PRIORITY

4fb06f0

Re-merge INITIAL_PRIORITY and PRIORITY, per Kazuho and Lucas

9ec6b29

Rework error layout

1b22092

More error shuffling

1920b13

MikeBishop force-pushed the http/not_excusive branch from b220a6c to 1920b13 Compare December 3, 2018 21:46

MikeBishop changed the title ~~Define INITIAL_PRIORITY frame and remove exclusive dependencies~~ Define initial PRIORITY frame and remove exclusive dependencies Dec 3, 2018

Merge remote-tracking branch 'origin/master' into http/not_excusive

0141a42

Limit references to existing elements

9acfcf6

kazuho reviewed Dec 5, 2018

View reviewed changes

Explicitly prohibit invalid PET values

c66e724

MikeBishop merged commit 1bc6191 into master Dec 6, 2018

dtikhonov mentioned this pull request Dec 12, 2018

Use less ambiguous language wrt PRIORITY on req stream #2126

Merged

MikeBishop deleted the http/not_excusive branch February 6, 2019 00:09

dtikhonov mentioned this pull request Mar 7, 2019

PRIORITY frame on control stream referencing unopened request stream #2502

Closed

ianswett mentioned this pull request May 17, 2019

HTTP/3 with strict priorities #2700

Closed

rmarx mentioned this pull request May 18, 2019

HTTP/3 Zero-weighting #2723

Closed

kazuho mentioned this pull request Jun 12, 2019

Bring back the Exclusive Flag #2781

Merged

Define initial PRIORITY frame and remove exclusive dependencies #2075

Define initial PRIORITY frame and remove exclusive dependencies #2075

Conversation

MikeBishop commented Nov 29, 2018 • edited Loading

MikeBishop commented Nov 30, 2018

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop commented Nov 30, 2018

RyanTheOptimist commented Dec 1, 2018

kazuho commented Dec 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPardue Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

LPardue Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPardue left a comment

Choose a reason for hiding this comment

MikeBishop commented Dec 3, 2018

kazuho commented Dec 3, 2018

kazuho left a comment

Choose a reason for hiding this comment

LPardue commented Dec 3, 2018

MikeBishop commented Dec 4, 2018

dtikhonov commented Dec 4, 2018

MikeBishop commented Dec 4, 2018

dtikhonov commented Dec 4, 2018

kazuho commented Dec 4, 2018

MikeBishop commented Dec 4, 2018

kazuho commented Dec 4, 2018 • edited Loading

MikeBishop commented Dec 5, 2018

kazuho commented Dec 5, 2018

MikeBishop commented Dec 5, 2018

kazuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop commented Nov 29, 2018 •

edited

Loading

LPardue Dec 2, 2018 •

edited

Loading

LPardue Dec 2, 2018 •

edited

Loading

kazuho commented Dec 4, 2018 •

edited

Loading