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

Move QPACK control instructions to dedicated streams #1238

Merged
merged 3 commits into from May 15, 2018

Conversation

afrind
Copy link
Contributor

@afrind afrind commented Mar 19, 2018

  1. Give QPACK encoders and decoders their own unidirectional streams (hardcoded to 6,7,10,11 for now)
  2. Define simple framing for encoder stream
  3. Move HEADER_ACK from HTTP over QUIC frame to QPACK instruction
  4. Introduce Table State Synchronize instruction

Fixes #1121
Fixes #1122

@martinthomson
Copy link
Member

Rather than hard-code, I would greatly prefer that this used a stream header.

@afrind
Copy link
Contributor Author

afrind commented Mar 19, 2018

Can you explain the basis of your preference? A client has no other unidirectional streams defined in HTTP over QUIC, and a server must process/acknowledge at least one header block before opening any push streams. It is further very unlikely the server would open a push stream without first encoding a header block that inserts at least one header to the dynamic table. So in practice it's going to work out to these streams anyways.

How do you see the encode/decode streams as materially different from the control stream, which is hardcoded?

Aside: it occurs to me that a server could compress a PUSH_PROMISE a lot better if it compressed against the client's table, which of course doesn't work ;)

@MikeBishop
Copy link
Contributor

Looks reasonable. The source of the stream header is, I think, the discussion in #910 about eliminating references to particular stream IDs in the application layer. You can make a QUIC interface that depends only on receiving a stream and reading the bytes to figure out what to do with it, rather than needing to open / listen on particular streams by ID.

I think that model is important for things that could crop up mid-connection, but I don't feel as strongly about the header for streams that are unique and persistent.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -qpack labels Mar 20, 2018
@MikeBishop
Copy link
Contributor

BTW, I think you might have meant #1122 rather than #1120. #1120 is another PR that has already been merged.

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.

Rebased onto the changes in master, but kept the changes from the original PR.


Table updates can add a table entry, possibly using existing entries to avoid
transmitting redundant information. The name can be transmitted as a reference
to an existing entry in the static or the dynamic table or as a string literal.
For entries which already exist in the dynamic table, the full entry can also be
used by reference, creating a duplicate entry.

Each set of encoder insructions is prefaced by its length, encoded as a variable
length integer with an 8-bit prefix.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably specify that these need to be complete instructions (i.e. no truncation), and require an error if one of the length-prefixed blocks ends mid-instruction.

afrind and others added 3 commits May 14, 2018 17:11
1. Give QPACK encoders and decoders their own unidirectional streams (hardcoded to 6,7,10,11 for now)
2. Define simple framing for encoder stream
3. Move HEADER_ACK from HTTP over QUIC frame to QPACK instruction
4. Introduce Table State Synchronize instruction

Closes #1121 and #1122
@afrind afrind force-pushed the qpack-streams-and-framing branch from 6175998 to e8d22aa Compare May 15, 2018 00:14
@afrind afrind merged commit c44f012 into master May 15, 2018
@martinthomson martinthomson deleted the qpack-streams-and-framing branch May 30, 2018 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants