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

Stricter, clearer rules for Dynamic Table Size Update #1003

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

martinthomson
Copy link
Collaborator

As they relate to changes in SETTINGS_HEADER_TABLE_SIZE.

This is the strict, by-the-book interpretation. That is, if the value
changes, you have to send the update, even if it is pointless (that is,
you aren't changing the size of the table).

Note that I'm eliding the point about multiple instructions, which are
necessary if the decoder reduces the maximum below the current size.
The only nod in that direction is "at least one" and a reference to the
section that talks about the need for multiple updates.

This pulls in more of RFC 7541 than I might have liked, but we have to
deal with the integration somewhere and this seems like a reasonable
place to do that. It might have been better to put this text in a -bis
revision of RFC 7541, but we decided to leave that document as-is.

As they relate to changes in SETTINGS_HEADER_TABLE_SIZE.

This is the strict, by-the-book interpretation.  That is, if the value
changes, you have to send the update, even if it is pointless (that is,
you aren't changing the size of the table).

Note that I'm eliding the point about multiple instructions, which are
necessary if the decoder reduces the maximum below the current size.
The only nod in that direction is "at least one" and a reference to the
section that talks about the need for multiple updates.

This pulls in more of RFC 7541 than I might have liked, but we have to
deal with the integration somewhere and this seems like a reasonable
place to do that.  It might have been better to put this text in a -bis
revision of RFC 7541, but we decided to leave that document as-is.
@wtarreau
Copy link
Contributor

LGTM, this is a nice clarification that removes the possibility of a different interpretation that's known to have caused interoperability issues (nghttp2 vs haproxy). Let's merge it!

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I agree with Willy, this is a really good clarification.

@Lukasa Lukasa merged commit 7741c1e into httpwg:main Dec 23, 2021
Copy link

@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.

I think this PR is in the correct direction.

Endpoints have to process instructions using dynamic table, even when they advertise SETTINGS_MAX_HEADER_LIST_SIZE of zero. That is because the default is 4096. We did fix this problem in QPACK, but HPACK had already shipped.

OTOH, I have concerns regarding new text suggesting Dynamic Table Update Size be sent even when there is no change in the size.

encoder after a change in SETTINGS_HEADER_TABLE_SIZE starts with at least one Dynamic
Table Size Update instruction; see <xref target="COMPRESSION" section="4.2"/>. An
encoder sends a Dynamic Table Size Update instruction after acknowledging a change of
SETTINGS_HEADER_TABLE_SIZE even if it is not changing the size of the dynamic table or
Copy link

@kazuho kazuho Dec 23, 2021

Choose a reason for hiding this comment

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

Is this correct?

Quoting from RFC 7541 section 4.2: A change in the maximum size of the dynamic table is signaled via a dynamic table size update (see Section 6.3). This dynamic table size update MUST occur at the beginning of the first header block following the change to the dynamic table size. In HTTP/2, this follows a settings acknowledgment (see Section 6.5.3 of [HTTP2]).

This paragraph of RFC 7541 starts "a change," but if the an HTTP/2 using an HPACK encoder with a dynamic table of size 4,096 (the default) and receives SETTINGS_MAX_HEADER_LIST_SIZE of 4096, it is not changing the size of the dynamic table.

Interoperability-wise, do existing servers respond with a Dynamic Table Update Size instruction when the client sends SETTINGS_MAX_HEADER_LIST_SIZE no less than 4096? It seems to me that this newly added text suggest such behavior is required, but I'm not sure if many servers behave as such. Unless we have such confirmation, it would be dangerous to indicate that clients might complain under such circumstance.

@kazuho
Copy link

kazuho commented Dec 23, 2021

Oh I see this PR already being merged. Opened #1004.

martinthomson added a commit that referenced this pull request Dec 24, 2021
martinthomson added a commit that referenced this pull request Dec 24, 2021
@martinthomson martinthomson linked an issue Dec 24, 2021 that may be closed by this pull request
martinthomson added a commit that referenced this pull request Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants