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

Jump to bottom

x/net/http2: missing SETTINGS_HEADER_TABLE_SIZE support #29356

Closed
rs opened this issue Dec 20, 2018 · 5 comments
Closed

x/net/http2: missing SETTINGS_HEADER_TABLE_SIZE support #29356

rs opened this issue Dec 20, 2018 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@rs
Copy link
Contributor

rs commented Dec 20, 2018 •
edited

There are TODO comments in the code about SETTINGS_HEADER_TABLE_SIZE support. Proper would:

  • Give the ability to setup a max table size at http2.Transport and http2.Server
  • Send the max table size in the settings frame
  • Apply received SETTINGS_HEADER_TABLE_SIZE capped by the max table size defined

I have a working implementation. I need to write tests and I will submit it.

Cc @bradfitz
@rs rs changed the title x x/net/http2: missing SETTINGS_HEADER_TABLE_SIZE support Dec 20, 2018
@gopherbot
Copy link

Change https://golang.org/cl/155877 mentions this issue: http2: add SETTINGS_HEADER_TABLE_SIZE support

@katiehockman katiehockman changed the title x/net/http2: missing SETTINGS_HEADER_TABLE_SIZE support net/http2: missing SETTINGS_HEADER_TABLE_SIZE support Jan 2, 2019
@katiehockman katiehockman added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 2, 2019
@katiehockman katiehockman added this to the Go1.13 milestone Jan 2, 2019
@rs
Copy link
Contributor Author

rs commented Jan 2, 2019

@katiehockman: all other issues related to x/net/http2 are prefixed with x/. What drove your decision to remove x/ here?

@katiehockman katiehockman changed the title net/http2: missing SETTINGS_HEADER_TABLE_SIZE support x/net/http2: missing SETTINGS_HEADER_TABLE_SIZE support Jan 2, 2019
@katiehockman
Copy link
Contributor

Just a mistake. Thanks for letting me know. Updated it.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

Change https://go.dev/cl/435899 mentions this issue: http2: add SETTINGS_HEADER_TABLE_SIZE support

@elindsey
Copy link

This patch looked close to landing, but discussion stalled out. I've rebased the patch from CL 155877, addressed the last piece of feedback I saw from Brad (splitting the MaxHeaderTableSize setting into two settings), added an additional test and published as a new CL.

@neild or @bradfitz if you have time, I'd very much appreciate a review.

@elindsey elindsey mentioned this issue Oct 5, 2022
Closed
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @neild
http2: add SETTINGS_HEADER_TABLE_SIZE support
8e16f9c 
Add support for handling of SETTINGS_HEADER_TABLESIZE in SETTINGS
frames.

Add http2.Transport.MaxDecoderHeaderTableSize to set the advertised
table size for new client connections. Add
http2.Transport.MaxEncoderHeaderTableSize to cap the accepted size for
new client connections.

Add http2.Server.MaxDecoderHeaderTableSize and MaxEncoderHeaderTableSize
to do the same on the server.

Fixes golang/go#29356
Fixes golang/go#56054

Change-Id: I16ae0f84b8527dc1e09dfce081e9f408fd514513
Reviewed-on: https://go-review.googlesource.com/c/net/+/435899
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Joedian Reid <joedian@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
@golang golang locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@elindsey @rs @rsc @andybons @gopherbot @katiehockman