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: cancelling a request from the Client doesn't release its stream #21543

Closed
BenPope opened this issue Aug 20, 2017 · 3 comments
Closed

x/net/http2: cancelling a request from the Client doesn't release its stream #21543

BenPope opened this issue Aug 20, 2017 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@BenPope
Copy link

BenPope commented Aug 20, 2017 •
edited

This is the second half of issue #21229 closed with change https://golang.org/cl/53250. The leak isn't fixed because the stream is still referenced by rl.activeRes.

What did you do?

  • An http2 client makes a request to a server with a cancellable context
  • The server sends a response but waits for req->Context().Done()
  • The client cancels its request context
  • The server cleans up its stream
  • The client leaks its stream

System details

go version go1.8.3 linux/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ben/development"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build098570469=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
GOROOT/bin/go version: go version go1.8.3 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.8.3 X:framepointer
uname -sr: Linux 4.12.4-041204-generic
LSB Version:	core-9.20160110ubuntu5-amd64:core-9.20160110ubuntu5-noarch:printing-9.20160110ubuntu5-amd64:printing-9.20160110ubuntu5-noarch:security-9.20160110ubuntu5-amd64:security-9.20160110ubuntu5-noarch
Distributor ID:	Ubuntu
Description:	Ubuntu 17.04
Release:	17.04
Codename:	zesty
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.24-9ubuntu2.2) stable release version 2.24, by Roland McGrath et al.
gdb --version: GNU gdb (Ubuntu 7.12.50.20170314-0ubuntu1.1) 7.12.50.20170314-git

@tombergan
@jsoriano jsoriano mentioned this issue Aug 22, 2017
Open
@tombergan tombergan self-assigned this Aug 24, 2017
@tombergan tombergan added this to the Go1.10 milestone Aug 24, 2017
@tombergan
Copy link
Contributor

Note to self, related to #22413: It is confusing to me that transport.go has both cc.streams and rl.activeRes. The only purpose of the later is to check if we have any idle streams. I believe rl.activeRes is redundant and can be removed, to be replaced with cc.streams.

Then, we need to be more careful about when a stream is removed from cc.streams. The simplest approach, also suggested in #22413, is the following: a stream is removed from cc.streams atomically with the user calling Response.Body.Close(), except if the request fails or if Response.Body is noBody, in which case, the stream should be removed immediately before RoundTrip returns.

@gopherbot
Copy link

Change https://golang.org/cl/80137 mentions this issue: http2: fix leak in activeRes by removing activeRes

gopherbot pushed a commit to golang/net that referenced this issue Nov 28, 2017
@tombergan
http2: fix leak in activeRes by removing activeRes
e6a4aa3 
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>
@gopherbot
Copy link

Change https://golang.org/cl/81276 mentions this issue: net/http: update bundled http2

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@tombergan
http2: fix leak in activeRes by removing activeRes
2f784bb 
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>
@golang golang locked and limited conversation to collaborators Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
3 participants
@BenPope @gopherbot @tombergan