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

Support very large headers #555

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Conversation

nox
Copy link
Contributor

@nox nox commented Aug 23, 2021

This completely refactors how headers are hpack-encoded.

Instead of trying to be clever, constructing frames on the go
while hpack-encoding, we just make a blob of all the
hpack-encoded headers first, and then we split that blob
in as many frames as necessary.

@nox nox force-pushed the support-very-large-headers branch 3 times, most recently from 984bcae to 7a87f24 Compare August 24, 2021 09:59
@ipostelnik
Copy link

Where does it check that total header size doesn't exceed SETTINGS_MAX_HEADER_LIST_SIZE?

@nox
Copy link
Contributor Author

nox commented Aug 24, 2021

Pretty sure it doesn't check that when encoding, note that it didn't beforehand either.

@nox nox force-pushed the support-very-large-headers branch from 7a87f24 to 90ba520 Compare August 24, 2021 14:28
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This looks nice and clean to me! Perhaps a test in h2-tests/codec_write about sending a header that would have been too big before?

@LPardue
Copy link
Contributor

LPardue commented Sep 3, 2021

There's a risk with enforcing SETTINGS_MAX_HEADER_LIST_SIZE when sending. The input may come from some other component or system that doesn't have such limits, and/or may not be able to change they header list to fit that size. An H2 library that enforces the rule can cause problems, so if it's ever implemented it probably needs to be optional.

This completely refactors how headers are hpack-encoded.

Instead of trying to be clever, constructing frames on the go
while hpack-encoding, we just make a blob of all the
hpack-encoded headers first, and then we split that blob
in as many frames as necessary.
@nox nox force-pushed the support-very-large-headers branch from 90ba520 to dc2f331 Compare September 8, 2021 08:18
@nox
Copy link
Contributor Author

nox commented Sep 8, 2021

This looks nice and clean to me! Perhaps a test in h2-tests/codec_write about sending a header that would have been too big before?

I added a 32KiB header in the existing test.

@seanmonstar seanmonstar merged commit 61b4f8f into master Sep 8, 2021
@seanmonstar seanmonstar deleted the support-very-large-headers branch September 8, 2021 17:20
nox added a commit that referenced this pull request Sep 13, 2021
nox added a commit that referenced this pull request Sep 13, 2021
seanmonstar pushed a commit that referenced this pull request Sep 13, 2021
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.

4 participants