Skip to content

Commit e488168

Browse files
committed
http2: fix request reservation when StrictMaxConcurrentStreams is enabled
Fix regression introduced in CL 617655. The awaitOpenSlotForStreamLocked() should not take into consideration reserved requests as they have no presence in the connection. Fixes golang/go#70809 Change-Id: Ia23968189cbdb44dae860a1de9d32700115b28b4
1 parent 8da7ed1 commit e488168

File tree

2 files changed

+48
-2
lines changed

2 files changed

+48
-2
lines changed

http2/transport.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1786,7 +1786,7 @@ func (cs *clientStream) cleanupWriteRequest(err error) {
17861786
close(cs.donec)
17871787
}
17881788

1789-
// awaitOpenSlotForStreamLocked waits until len(streams) < maxConcurrentStreams.
1789+
// awaitOpenSlotForStreamLocked waits until len(streams) + pendingResets < maxConcurrentStreams.
17901790
// Must hold cc.mu.
17911791
func (cc *ClientConn) awaitOpenSlotForStreamLocked(cs *clientStream) error {
17921792
for {
@@ -1800,7 +1800,7 @@ func (cc *ClientConn) awaitOpenSlotForStreamLocked(cs *clientStream) error {
18001800
return errClientConnUnusable
18011801
}
18021802
cc.lastIdle = time.Time{}
1803-
if cc.currentRequestCountLocked() < int(cc.maxConcurrentStreams) {
1803+
if int64(len(cc.streams)+cc.pendingResets) < int64(cc.maxConcurrentStreams) {
18041804
return nil
18051805
}
18061806
cc.pendingRequests++

http2/transport_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5425,6 +5425,52 @@ func TestIssue67671(t *testing.T) {
54255425
}
54265426
}
54275427

5428+
// Issue 70809: when StrictMaxConcurrentStreams enabled ClientConn.ReserveNewRequest() causes stalls in request processing
5429+
func TestIssue70809(t *testing.T) {
5430+
ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {}, func(s *Server) {
5431+
s.MaxConcurrentStreams = 1
5432+
})
5433+
tr := &Transport{
5434+
TLSClientConfig: tlsConfigInsecure,
5435+
AllowHTTP: true,
5436+
StrictMaxConcurrentStreams: true,
5437+
}
5438+
5439+
var opt RoundTripOpt
5440+
5441+
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
5442+
defer cancel()
5443+
5444+
get := func() int {
5445+
req, _ := http.NewRequestWithContext(ctx, "GET", ts.URL, nil)
5446+
res, err := tr.RoundTripOpt(req, opt)
5447+
if err != nil {
5448+
t.Error(err)
5449+
}
5450+
res.Body.Close()
5451+
return res.StatusCode
5452+
}
5453+
5454+
if get() != 200 {
5455+
t.Fatal("first request failed")
5456+
}
5457+
5458+
// Do not dial new connections.
5459+
opt.OnlyCachedConn = true
5460+
5461+
var wg sync.WaitGroup
5462+
for i := 0; i < 10; i++ {
5463+
wg.Add(1)
5464+
go func() {
5465+
defer wg.Done()
5466+
if get() != 200 {
5467+
t.Errorf("request failed")
5468+
}
5469+
}()
5470+
}
5471+
wg.Wait()
5472+
}
5473+
54285474
func TestTransport1xxLimits(t *testing.T) {
54295475
for _, test := range []struct {
54305476
name string

0 commit comments

Comments
 (0)