http2: fix leak in activeRes by removing activeRes
AFAICT, activeRes serves no real purpose. It is used in just two ways:
- To reduce the number of calls to closeIfIdle, which reduces the number
of acquires of cc.mu when there are many concurrent streams. I dug
through the CL history and could not find any benchmarks showing that
this is necessary.
- To avoid redundant calls to cs.bufPipe.CloseWithError(err) when a read
loop is shutdown. This is unnecessary, since redundant CloseWithError
calls are ignored.
Since there isn't a good reason to have activeRes, the simplest way to
fix the leak is to remove activeRes entirely.
Updates golang/go#21543
Change-Id: I1d1d2dc6c946425a2772c8bf71436707021ac269
Reviewed-on: https://go-review.googlesource.com/80137
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/http2/transport.go b/http2/transport.go
index 6346dd6..e396352 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1376,17 +1376,12 @@
// clientConnReadLoop is the state owned by the clientConn's frame-reading readLoop.
type clientConnReadLoop struct {
cc *ClientConn
- activeRes map[uint32]*clientStream // keyed by streamID
closeWhenIdle bool
}
// readLoop runs in its own goroutine and reads and dispatches frames.
func (cc *ClientConn) readLoop() {
- rl := &clientConnReadLoop{
- cc: cc,
- activeRes: make(map[uint32]*clientStream),
- }
-
+ rl := &clientConnReadLoop{cc: cc}
defer rl.cleanup()
cc.readerErr = rl.run()
if ce, ok := cc.readerErr.(ConnectionError); ok {
@@ -1441,10 +1436,8 @@
} else if err == io.EOF {
err = io.ErrUnexpectedEOF
}
- for _, cs := range rl.activeRes {
- cs.bufPipe.CloseWithError(err)
- }
for _, cs := range cc.streams {
+ cs.bufPipe.CloseWithError(err) // no-op if already closed
select {
case cs.resc <- resAndError{err: err}:
default:
@@ -1522,7 +1515,7 @@
}
return err
}
- if rl.closeWhenIdle && gotReply && maybeIdle && len(rl.activeRes) == 0 {
+ if rl.closeWhenIdle && gotReply && maybeIdle {
cc.closeIfIdle()
}
}
@@ -1578,9 +1571,6 @@
// (nil, nil) special case. See handleResponse docs.
return nil
}
- if res.Body != noBody {
- rl.activeRes[cs.ID] = cs
- }
cs.resTrailer = &res.Trailer
cs.resc <- resAndError{res: res}
return nil
@@ -1919,7 +1909,6 @@
rl.closeWhenIdle = true
}
cs.bufPipe.closeWithErrorAndCode(err, code)
- delete(rl.activeRes, cs.ID)
select {
case cs.resc <- resAndError{err: err}:
@@ -2046,7 +2035,6 @@
cs.bufPipe.CloseWithError(err)
cs.cc.cond.Broadcast() // wake up checkResetOrDone via clientStream.awaitFlowControl
}
- delete(rl.activeRes, cs.ID)
return nil
}
