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

net/http: HTTP/2 configuration API #67813

Open
Tracked by #67810
neild opened this issue Jun 4, 2024 · 12 comments
Open
Tracked by #67810

net/http: HTTP/2 configuration API #67813

neild opened this issue Jun 4, 2024 · 12 comments

Comments

@neild
Copy link
Contributor

neild commented Jun 4, 2024

This issue is part of a project to move x/net/http2 into std: #67810

Configuring HTTP/2-specific protocol options currently requires users to import the golang.org/x/net/http2 package and call http2.ConfigureServer or http2.ConfigureTransports.

Configuring options in this fashion has the side effect of replacing the bundled HTTP/2 implementation in net/http with the one in golang.org/x/net/http2.

I propose adding HTTP/2 configuration options to net/http, permitting HTTP/2 servers and clients to be configured without importing an external package and separating configuration from version selection.

Many of the HTTP/2 settings are identical for server and client connections. For example, the MaxDecoderHeaderTableSize field of http2.Server and http2.Transport sets the SETTINGS_HEADER_TABLE_SIZE setting sent to the peer. We will unify these settings into a single configuration struct, and add this configuration to http.Server and http.Transport.

type HTTP2Config struct {
	// MaxConcurrentStreams optionally specifies the number of
	// concurrent streams that a peer may have open at a
	// time (SETTINGS_MAX_CONCURRENT_STREAMS).
	// If zero, MaxConcurrentStreams defaults to at least 100.
	MaxConcurrentStreams uint32

	// MaxDecoderHeaderTableSize optionally specifies the http2
	// SETTINGS_HEADER_TABLE_SIZE to send in the initial settings frame. It
	// informs the remote endpoint of the maximum size of the header compression
	// table used to decode header blocks, in octets. If zero, the default value
	// of 4096 is used.
	MaxDecoderHeaderTableSize uint32

	// MaxEncoderHeaderTableSize optionally specifies an upper limit for the
	// header compression table used for encoding request headers. Received
	// SETTINGS_HEADER_TABLE_SIZE settings are capped at this limit. If zero,
	// the default value of 4096 is used.
	MaxEncoderHeaderTableSize uint32

	// MaxReadFrameSize optionally specifies the largest frame
	// this endpoint is willing to read (SETTINGS_MAX_FRAME_SIZE).
	// A valid value is between 16k and 16M, inclusive.
	// If zero or otherwise invalid, a default value is used.
	MaxReadFrameSize uint32

	// MaxUploadBufferPerConnection is the size of the initial flow
	// control window for each connection. The HTTP/2 spec does not
	// allow this to be smaller than 65535 or larger than 2^32-1.
	// If the value is outside this range, a default value will be
	// used instead.
	MaxUploadBufferPerConnection int32

	// MaxUploadBufferPerStream is the size of the initial flow control
	// window for each stream. The HTTP/2 spec does not allow this to
	// be larger than 2^32-1. If the value is zero or larger than the
	// maximum, a default value will be used instead.
	MaxUploadBufferPerStream int32

	// SendPingTimeout is the timeout after which a health check using a ping
	// frame will be carried out if no frame is received on the connection.
	// If zero, no health check is performed.
	SendPingTimeout time.Duration

	// PingTimeout is the timeout after which the connection will be closed
	// if a response to a ping is not received.
	// If zero, a default of 15 seconds is used.
	PingTimeout time.Duration

	// WriteByteTimeout is the timeout after which a connection will be
	// closed if no data can be written to it. The timeout begins when data is
	// available to write, and is extended whenever any bytes are written.
	WriteByteTimeout time.Duration

	// PermitProhibitedCipherSuites, if true, permits the use of
	// cipher suites prohibited by the HTTP/2 spec.
	PermitProhibitedCipherSuites bool

	// CountError, if non-nil, is called on HTTP/2 errors.
	// It's intended to increment a metric for monitoring, such
	// as an expvar or Prometheus metric.
	// The errType consists of only ASCII word characters.
	CountError func(errType string)
}

type Server struct { // contains unchanged fields
	HTTP2 HTTP2Config
}

type Transport struct { // contains unchanged fields
	HTTP2 HTTP2Config
}

The SendPingTImeout and PingTimeout fields assume #67812 is accepted, and the WriteByteTimeout field assumes #67811 is accepted.

The http2.Transport.StrictMaxConcurrentStreams field controls whether a new connection should be opened to a server if an existing connection has exceeded its stream limit. For example, if an HTTP/2 server advertises a stream limit of 100 concurrent streams, then the 101st concurrent stream opened by the client will block waiting for an existing stream to complete when StrictMaxConcurrentStreams is true, or create a new connection when it is false. There is no equivalent to this setting for HTTP/1 connections, which only support a single concurrent request per connection. We will add this setting to http.Transport, since it could be used to configure HTTP/3 connections (if and when we support HTTP/3):

type Transport struct { // contains unchanged fields
	// StrictMaxConcurrentRequests controls whether an HTTP/2 server's
	// concurrency limit should be respected globally.
	// If true, new requests sent when an connection's concurrency limit has been exceeded
	// will block until an existing request completes.
	// If false, an additional connection will be opened if all existing connections are at their limit.
	StrictMaxConcurrentRequests bool
}
@neild neild added the Proposal label Jun 4, 2024
@gopherbot gopherbot added this to the Proposal milestone Jun 4, 2024
@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 20, 2024

Why are the types uncommon (for Go APIs) sizes like uint32? Can they all be int?
Otherwise this seems fine.

@neild
Copy link
Contributor Author

neild commented Jun 20, 2024

The proposal reuses the types from the x/net/http2 Server and Transport.

HTTP/2 settings are defined as 32-bit unsigned integers; I don't know for sure, but I'd guess that's the reason for the uint32s. I don't see a problem with defining these as ints, although the extra range isn't going to be used. 24 more bytes in the Server and Transport structs isn't going to be noticeable, given that you're not expected to have a lot of either.

@rsc
Copy link
Contributor

rsc commented Jun 26, 2024

It seems like now that this has outgrown being an HTTP/2 guts package, we should probably use Go types (int or else int64 if it really matters) instead of wire types. Otherwise this seems fine.

@rsc
Copy link
Contributor

rsc commented Jun 27, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is #67813 (comment), except using ints instead of uint32 etc.

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is #67813 (comment), except using ints instead of uint32 etc.

@rsc
Copy link
Contributor

rsc commented Jul 31, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is #67813 (comment), except using ints instead of uint32 etc.

@rsc rsc changed the title proposal: net/http: HTTP/2 configuration API net/http: HTTP/2 configuration API Jul 31, 2024
@rsc rsc modified the milestones: Proposal, Backlog Jul 31, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/602175 mentions this issue: net/http: add HTTP2Config

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/607255 mentions this issue: http2: add support for net/http HTTP2 config field

@neild
Copy link
Contributor Author

neild commented Aug 26, 2024

A minor change to rename two fields:

MaxUploadBufferPerConnection is now MaxReceiveBufferPerConnection.
MaxUploadBufferPerStream is now MaxReceiveBufferPerStream.

These fields originate as http2.Server fields, where "upload" unambiguously refers to the flow control window used to limit the rate at which the peer sends data. As part of a common struct configuring client and server, I think it's clearer for this field to consistently control the peer's flow control window, in which case "receive buffer" is more accurate than "upload buffer".

gopherbot pushed a commit that referenced this issue Aug 29, 2024
Add a field to Server and Transport containing HTTP/2 configuration
parameters.

This field will have no effect until golang.org/x/net/http2 is updated
to make use of it, and h2_bundle.go is updated with the new http2
package.

For #67813

Change-Id: I81d7f8e9ddea78f9666383983aec43e3884c13ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/602175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
gopherbot pushed a commit to golang/net that referenced this issue Sep 25, 2024
For golang/go#67813

Change-Id: I6b7f857d6ed250ba8b09649730980a91b3e8d7e9
Reviewed-on: https://go-review.googlesource.com/c/net/+/607255
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615875 mentions this issue: net/http: add Transport.StrictMaxConcurrentRequests

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615895 mentions this issue: http2: add support for net/http Transport.StrictMaxConcurrentRequests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

4 participants
@neild @rsc @gopherbot and others