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

Separate HEADERS+PRIORITY #99

Closed
jasnell opened this issue May 21, 2013 · 8 comments
Closed

Separate HEADERS+PRIORITY #99

jasnell opened this issue May 21, 2013 · 8 comments

Comments

@jasnell
Copy link
Contributor

jasnell commented May 21, 2013

With regards to the discussion over stream re-prioritization, I suggest:

  1. Drop the HEADERS+PRIORITY frame type.
  2. Create a new separate PRIORITY frame type whose payload is the Priority value, no frame-specific flags.

The PRIORITY frame becomes the only way to set/change the priority for a stream.

If it is necessary to allow an endpoint to establish the priority of stream prior to actually initiating the stream, we can allow sending a PRIORITY frame before the initial HEADERS frame. Doing so would effectively reserve the stream id (in the same general manner PUSH_PROMISE does).

The advantages of this approach are:

  1. It eliminates any possible confusion and complexity about when to use HEADERS+PRIORITY vs. HEADERS
  2. It provides a single way of setting/change stream priority (as opposed to using HEADERS+PRIORITY plus a separate CHANGE-PRIORITY frame)
@martinthomson
Copy link
Collaborator

I made the same suggestion to a few folks. It wasn't that popular.

A number of things to consider:

  1. This adds 4 more bytes to the common case. That was the primary reason for objections.
  2. You need to specify a default priority.
  3. It is necessary (not optional) to have it sent prior to sending out the headers. A server typically processes requests as soon as seeing headers. Sending priority afterwards would lead to suboptimal performance.

PRIORITY would be a stream-related frame, sent on the stream that is being reprioritized. This makes it very different from PUSH_PROMISE.

@jasnell
Copy link
Contributor Author

jasnell commented May 21, 2013

  1. I don't see 4 bytes as being critical but noted.
  2. We already allow streams to be created using only a HEADERS frame, so a default priority is already assumed unless we specifically require HEADERS+PRIORITY to initiate streams (which we currently only do in the semantic layer, not the framing layer).
  3. Noted.

@martinthomson
Copy link
Collaborator

Correct, it's the semantic layer that requires the use of HEADERS+PRIORITY. For HTTP my interpretation is that this is strong enough that we don't require a default priority.

@jpinner
Copy link

jpinner commented May 25, 2013

I would like to see this change.

  1. Don't agree that it adds 4 bytes in the common case. I could easily argue that by defaulting everything to the lowest priority it saves 4 bytes in the common case.
  2. Since most existing HTTP implementations do not specify a priority for requests HTTP/1.x<->HTTP/2 proxies etc. will already have to assign a default priority.
  3. Send it before with the same requirements that the stream identifier in the frame must be for an open stream or the "next valid identifier" as in the PUSH_PROMISE up to the max concurrent stream limit (2 open streams with a max concurrent limit of 10 for example would allow you to send PRIORITY frames for the next 8 unopened streams).

@jpinner
Copy link

jpinner commented May 28, 2013

From the wg list:

  1. Remove frame 0x08 HEADERS.
  2. Rename frame 0x01 from HEADERS+PRIORITY to HEADERS.
  3. Add flag 0x04 PRIORITY to the now-renamed HEADERS frame to indicate that the first 4 bytes of the frame contains a priority field.
  4. Indicate that HEADERS is the only frame that may create a stream in section 3.4.1 (Stream Creation).
  5. Add a new frame 0x02 PRIORITY that is 4 bytes long and must be sent only on open streams.
  6. Add text to section 3.4.2 (Stream Priority) that have to do with priority assignment during stream creation (with a HEADERS frame) and re-prioritization (with a PRIORITY frame).

@willchan willchan mentioned this issue Jun 4, 2013
@mnot
Copy link
Member

mnot commented Jun 13, 2013

Discussed at SF Interim; pending proposal from Layering TF.

@martinthomson
Copy link
Collaborator

@jasnell will propose text for this. This is going to be completely opposite to the original issue header. HEADERS will be the only frame. HEADERS+PRIORITY will go away. The new HEADERS frame will include a flag, which when set indicates that there is a four byte priority. Priority can be changed on every HEADERS frame. If the server includes PRIORITY then it is the priority that the server has decided to use - which is advisory only.

@martinthomson
Copy link
Collaborator

Done in #130.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants