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.

Sign up for GitHub

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

http2: limit client initial MAX_CONCURRENT_STREAMS #73

Closed
wants to merge 1 commit into from
Closed

http2: limit client initial MAX_CONCURRENT_STREAMS #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions http2/transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ const (
transportDefaultStreamMinRefresh = 4 << 10

defaultUserAgent = "Go-http-client/2.0"

// initialMaxConcurrentStreams is a connections maxConcurrentStreams until
// it's received servers initial SETTINGS frame, which corresponds with the
// spec's minimum recommended value.
initialMaxConcurrentStreams = 100

// defaultMaxConcurrentStreams is a connections default maxConcurrentStreams
// if the server doesn't include one in its initial SETTINGS frame.
defaultMaxConcurrentStreams = 1000
)

// Transport is an HTTP/2 Transport.
Expand Down Expand Up @@ -237,6 +246,7 @@ type ClientConn struct {
inflow flow // peer's conn-level flow control
closing bool
closed bool
seenSettings bool // true if we've seen a settings frame, false otherwise
wantSettingsAck bool // we sent a SETTINGS frame and haven't heard back
goAway *GoAwayFrame // if non-nil, the GoAwayFrame we received
goAwayDebug string // goAway frame's debug data, retained as a string
Expand Down Expand Up @@ -634,10 +644,10 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro
tconn: c,
readerDone: make(chan struct{}),
nextStreamID: 1,
maxFrameSize: 16 << 10, // spec default
initialWindowSize: 65535, // spec default
maxConcurrentStreams: 1000, // "infinite", per spec. 1000 seems good enough.
peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead.
maxFrameSize: 16 << 10, // spec default
initialWindowSize: 65535, // spec default
maxConcurrentStreams: initialMaxConcurrentStreams, // "infinite", per spec. Use a smaller value until we have received server settings.
Copy link

Choose a reason for hiding this comment

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

Are there any restrictions that preventing setting maxConcurrentStreams in Transport configurations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could be done but it only has meaning for http2

Copy link

Choose a reason for hiding this comment

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

it only has meaning for http2

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Transport configurations come from: https://pkg.go.dev/net/http#Transport which is used for http1 and http2, just flagging that adding an option for initial max concurrent streams would be http2 specific.

Copy link

Choose a reason for hiding this comment

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

By using https://pkg.go.dev/net/http#Transport with function https://pkg.go.dev/golang.org/x/net/http2#ConfigureTransports limited the connection reuse cases:

net/http2/transport.go

Lines 173 to 176 in ad29c8a

t2 := &Transport{
ConnPool: noDialClientConnPool{connPool},
t1: t1,
}

This may result in an http2: no cached connection was available error from the lines below:

net/http2/transport.go

Lines 476 to 480 in ad29c8a

cc, err := t.connPool().GetClientConn(req, addr)
if err != nil {
t.vlogf("http2: Transport failed to get client conn for %s: %v", addr, err)
return nil, err
}

Aren't you modifying the http2 package source code? Why shouldn't it have a MaxConcurrentStreams specific configuration anyway?

peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead.
streams: make(map[uint32]*clientStream),
singleUse: singleUse,
wantSettingsAck: true,
Expand Down Expand Up @@ -2319,12 +2329,14 @@ func (rl *clientConnReadLoop) processSettings(f *SettingsFrame) error {
return ConnectionError(ErrCodeProtocol)
}

var seenMaxConcurrentStreams bool
err := f.ForeachSetting(func(s Setting) error {
switch s.ID {
case SettingMaxFrameSize:
cc.maxFrameSize = s.Val
case SettingMaxConcurrentStreams:
cc.maxConcurrentStreams = s.Val
seenMaxConcurrentStreams = true
case SettingMaxHeaderListSize:
cc.peerMaxHeaderListSize = uint64(s.Val)
case SettingInitialWindowSize:
Expand Down Expand Up @@ -2356,6 +2368,17 @@ func (rl *clientConnReadLoop) processSettings(f *SettingsFrame) error {
return err
}

if !cc.seenSettings {
if !seenMaxConcurrentStreams {
// This was the servers initial SETTINGS frame and it
// didn't contain a MAX_CONCURRENT_STREAMS field so
// increase the number of concurrent streams this
// connection can establish to our default.
cc.maxConcurrentStreams = defaultMaxConcurrentStreams
}
cc.seenSettings = true
}

cc.wmu.Lock()
defer cc.wmu.Unlock()

Expand Down