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

Insufficiently enthusiastic about emitting header table size shrinkages. #11

Closed
Lukasa opened this issue Oct 23, 2015 · 8 comments
Closed
Labels

Comments

@Lukasa
Copy link
Member

Lukasa commented Oct 23, 2015

Whenever we shrink the table size in the encoder, we must emit a header table size update field. This is true even if we change the header table size multiple times: e.g., we shrink and then raise the header table size. Currently we don't do this, which means we're technically violating the RFC.

This came from the following comment on the ML:

  • If between headers decoder reduces the limit below size signaled by
    encoder, the encoder must first reduce the table size to the minimum
    it was between the frames or less (it can then increase it up to
    current limit).

As example of the last point:

[4k dynamic table size in use]
--> HEADERS
<-- SETTINGS(SETTINGS_HEADER_TABLE_SIZE=4k)
<-- SETTINGS(SETTINGS_HEADER_TABLE_SIZE=2k)
<-- SETTINGS(SETTINGS_HEADER_TABLE_SIZE=4k)
<-- SETTINGS(SETTINGS_HEADER_TABLE_SIZE=8k)
<-- SETTINGS(SETTINGS_HEADER_TABLE_SIZE=6k)
--> HEADERS

The second HEADERS must first reduce the dynamic table to at most
2k. It can then increase dynamic table size to up to 6k.

@Lukasa Lukasa added the bug label Oct 23, 2015
@Lukasa
Copy link
Member Author

Lukasa commented Oct 24, 2015

The safer policy here might be to emit a header-table-size changed field for every change, regardless of whether we shrank or increased the size of the header table.

@jimcarreer
Copy link
Contributor

Wouldn't this have to do with whatevers using the encoder object, such as

encoder.header_table_size = ...
//Just need table size change field
encoded = encoder.add(to_add={})
... send encoded / frame to remote host ...

IIRC we always encode a table size change when add is called and header_table_size has changed (even if it got smaller). Or am I missing something? Do we have to keep a 'history' of header_table_sizes that are saved and then all encoded at once when add is called?

@Lukasa
Copy link
Member Author

Lukasa commented Oct 24, 2015

@jimcarreer It would if we could guarantee that there was only going to be one change per headers frame. We cannot guarantee that: specifically, we cannot guarantee that a remote peer won't spew out many OPTIONS frames that regularly change the table size before we next emit a header block.

For this reason, yeah, I think we need to keep a history and then encoded them all at once.

@jimcarreer
Copy link
Contributor

Do we need to differentiate between a size change mandated by the local implementation over a change mandated by the remote implementation spewing out the OPTIONS frames? IE if we set it, we don't keep a history, if we specify that this is a change asked for by the remote host, we keep the history?

@Lukasa
Copy link
Member Author

Lukasa commented Oct 24, 2015

No, but only because the distinction between those two cases is implicit in the distinction between encoder and decoder.

The way this works is, a peer decides the maximum amount of resources it is willing to commit to decoding HPACK, and turns this into a value of SETTINGS_MAX_HEADER_TABLE_SIZE. It sends this setting to the other peer. The other peer receives that setting, and then selects how many resources it is willing to commit to encoding HPACK. It then sets that value in its HPACK encoder, which should then emit a table size changed field in the next header block it sends. The HPACK decoder should receive that value and change its header size accordingly.

This means that the HTTP/2 implementation should only ever explicitly set the size of its encoder, not its decoder. Its decoder should have its size informed by SETTINGS_MAX_HEADER_TABLE_SIZE. The HTTP/2 implementation should, however, ensure that the header table size on the decoder never exceeds SETTINGS_MAX_HEADER_TABLE_SIZE.

@jimcarreer
Copy link
Contributor

Ok, that makes sense. It shouldn't be to hard to track header size changes in the table, then encode them all at once in add(...). I can actually take ownership of this if you want, should be a nice easy task to get me back in the swing.

@Lukasa
Copy link
Member Author

Lukasa commented Oct 24, 2015

Yeah, I don't think this is tricky at all, just a thing we need to be keeping track of. I'm happy for you to do this work if you'd like to. =)

Lukasa added a commit that referenced this issue Oct 28, 2015
@Lukasa
Copy link
Member Author

Lukasa commented Nov 9, 2015

Fixed in 2.0.1.

@Lukasa Lukasa closed this as completed Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants