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

QPACK: stream cancellation only required when max dynamic table size > 0 #1747

Closed
afrind opened this issue Sep 14, 2018 · 12 comments
Closed
Assignees
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@afrind
Copy link
Contributor

afrind commented Sep 14, 2018

The stream cancellation instruction serves to help the encoder cleanup state related to the dynamic table, but a decoder that sets the maximum dynamic table size to 0 does not need to send them. Now that we omit header acknowledgement for blocks with no dynamic table references, this is the only use of the QPACK control streams for an implementation not using the dynamic table.

This is not to be confused with the encoder's current table size, which can be temporarily zero but raised later to the decoder's maximum.

@afrind afrind added the -qpack label Sep 14, 2018
@afrind afrind changed the title QPACK: stream cancellation only required when dynamic table size > 0 QPACK: stream cancellation only required when max dynamic table size > 0 Sep 14, 2018
@martinthomson
Copy link
Member

If you think of this as an optimization for the case where there is no dynamic table AND you are cancelling streams (i.e., an exceptional condition on an exceptional condition), this really gets exciting (not). Let them send cancellations.

@dtikhonov
Copy link
Member

Why send cancellations that serve no purpose? My vote is for a consistent spec.

@kazuho
Copy link
Member

kazuho commented Sep 17, 2018

I do not have a strong opinion on this, but I do think the proposal can be considered as a simplification.

The proposal allows endpoints that set maximum dynamic table size to zero to not even open a decoder stream. Maybe we could go one step further and forbid the creation of encoder stream if the maximum dynamic table size advertised by peer is zero.

Then, we can have simple HQ stacks that do not handle QPACK streams at all.

@dtikhonov
Copy link
Member

Amen.

@mikkelfj
Copy link
Contributor

That would be useful for constrained devices.

@bencebeky
Copy link
Contributor

There can be problems if SETTINGS_HEADER_TABLE_SIZE is allowed to be changed to zero mid-connection (not with initial SETTINGS), so maybe this proposal should also require SETTINGS_HEADER_TABLE_SIZE not to be changed (or at least not to be set to zero) other than in the initial settings frame.

Otherwise what would happen if an endpoint sets SETTINGS_HEADER_TABLE_SIZE to zero not at the beginning of the connection, then a stream is cancelled but the peer does not need to be modified, then SETTINGS_HEADER_TABLE_SIZE is changed back to non-zero? How will the peer know that the stream got cancelled?

Another consideration is that if an endpoint tries to close the decoder stream after changing SETTINGS_HEADER_TABLE_SIZE to zero mid-connection, there is no guarantee that the setting will reach the peer before the stream closure which is normally an error. Therefore decoder streams cannot be closed even in this case, leaving the connection with an unused stream.

Finally, I believe that the order between SETTINGS frames and data on the decoder stream is not guaranteed. Therefore it would need to be spelled out that a stream cancellation instruction received while SETTINGS_HEADER_TABLE_SIZE is zero must not be treated as an error (it might have been sent before the setting was changed).

That's why I think maybe it's easier not to allow SETTINGS_HEADER_TABLE_SIZE to be changed (at least to zero) after the initial SETTINGS frame.

@afrind
Copy link
Contributor Author

afrind commented Sep 17, 2018

@bencebeky : in HQ you can only send SETTINGS at the beginning of the connection, for all the reasons you describe.

@kazuho : my reasoning here was exactly to allow for simpler implementations that do not implement a dynamic table. I hadn't thought about not opening the streams, but I'm not sure it really matters - nothing will ever be sent on them so you can effectively ignore them.

@janaiyengar janaiyengar added the design An issue that affects the design of the protocol; resolution requires consensus. label Sep 17, 2018
@bencebeky
Copy link
Contributor

Sorry, I wasn't aware of this amazing restriction in HQ. Makes sense.

@martinthomson
Copy link
Member

So the biggest problem here is for a generic implementation of the encoder where it now needs to handle the expectation of stream cancellation acknowledgment specially. When I send a header block, I track the need for acknowledgment. Creating the tracking in a way that is robust against that is tricky.

@kazuho
Copy link
Member

kazuho commented Sep 18, 2018

@afrind

my reasoning here was exactly to allow for simpler implementations that do not implement a dynamic table.

👍

I hadn't thought about not opening the streams, but I'm not sure it really matters - nothing will ever be sent on them so you can effectively ignore them.

The issue is the additional state. You need to accept the creation of the decoder stream and you cannot reset it. For memory constrained devices, we could have QUIC stacks that support only a small number of concurrently open streams. Spending one stream for transmitting nothing would be an overhead.

@MikeBishop
Copy link
Contributor

@martinthomson, I'm not sure that you do. If the decoder's maximum size is zero, you can never insert to the dynamic table, and therefore can never emit a header block that references it. That means you'll never have sent a header block that expects an acknowledgement, since we already agreed not to ack static-only blocks.

@martinthomson
Copy link
Member

Right, that was the conclusion of the discussion in NYC.

afrind added a commit that referenced this issue Sep 27, 2018
…=0 (#1761)

* QPACK: don't send stream cancellation when SETTINGS_HEADER_TABLE_SIZE=0

Fixes: #1747

* s/does not/SHOULD NOT/

* Change 'SHOULD NOT send' to 'MAY omit'

* s/send/sending/
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
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. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

9 participants