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

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
Assignees
Milestone

Comments

@BenPope
Copy link

@BenPope BenPope commented Aug 20, 2017

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
@tombergan tombergan self-assigned this Aug 24, 2017
@tombergan tombergan added this to the Go1.10 milestone Aug 24, 2017
@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Nov 2, 2017

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 28, 2017

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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 30, 2017

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

@gopherbot gopherbot closed this in 7e394a2 Dec 1, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.