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

Closed
BenPope opened this issue Jul 31, 2017 · 5 comments
Closed
Assignees
Milestone

Comments

@BenPope
Copy link

@BenPope BenPope commented Jul 31, 2017

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 it's stream
package main

import (
	"context"
	"crypto/tls"
	"fmt"
	"net"
	"net/http"
	"os"
	"time"

	"golang.org/x/net/http2"
)

func assert(err error) {
	if err != nil {
		panic(err)
	}
}

type handler struct{}

func (h *handler) ServeHTTP(res http.ResponseWriter, req *http.Request) {
	res.WriteHeader(200)
	res.(http.Flusher).Flush()
	<-req.Context().Done()
}

func makeServer() *http.Server {
	httpServer := &http.Server{
		Addr: ":" + os.Args[1],
	}

	httpServer.ConnState = func(_ net.Conn, state http.ConnState) {
		if state == http.StateNew {
			fmt.Println("connections++")
		} else if state == http.StateClosed {
			fmt.Println("connections--")
		}
	}

	err := http2.ConfigureServer(httpServer, &http2.Server{})
	assert(err)

	httpServer.Handler = &handler{}

	go func() {
		err = httpServer.ListenAndServeTLS(os.Args[2], os.Args[3])
		assert(err)
	}()

	return httpServer
}

func makeRequest(httpClient *http.Client) context.CancelFunc {
	ctx, cancel := context.WithCancel(context.Background())
	request, err := http.NewRequest("GET", "https://localhost:"+os.Args[1], nil)
	assert(err)

	request = request.WithContext(ctx)
	response, err := httpClient.Do(request)
	assert(err)

	err = response.Body.Close()
	assert(err)

	return cancel
}

func makeClient() *http.Client {
	transport := &http.Transport{
		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
	}

	err := http2.ConfigureTransport(transport)
	assert(err)

	return &http.Client{Transport: transport}
}

func main() {
	if len(os.Args) != 4 {
		fmt.Println("Usage: ", os.Args[0], " <port> <cert> <key>")
		os.Exit(1)
	}

	httpServer := makeServer()
	time.Sleep(time.Second)
	httpClient := makeClient()

	for i := 0; i < 1001; i++ {
		cancel := makeRequest(httpClient)
		cancel()
		if i%50 == 0 {
			fmt.Println("Requests: ", i)
		}
	}

	err := httpServer.Close()
	assert(err)
}


What did you expect to see?

One connection should be created, since there are fewer than MaxConcurrentStreams at any given time.

What did you see instead?

Multiple connections are made to the server.

Suggested fix

The suggested fix in transport.go is in two parts:

  • Cancel the read by calling clientConnReadLoop.endStreamError during clientStream.awaitRequestCancel instead of just clientStream.bufPipe.CloseWithError
  • Remove the stream by calling something equivalent to ClientConn.forgetStreamID during clientStream.cancelStream - but without locking and unlocking the mutex twice.

E.g., the following diff for exposition:

diff --git a/http2/transport.go b/http2/transport.go
index 850d7ae..fc97566 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -221,7 +221,7 @@ type clientStream struct {
 // request to be done (any way it might be removed from the cc.streams
 // map: peer reset, successful completion, TCP connection breakage,
 // etc)
-func (cs *clientStream) awaitRequestCancel(req *http.Request) {
+func (cs *clientStream) awaitRequestCancel(req *http.Request, rl *clientConnReadLoop) {
        ctx := reqContext(req)
        if req.Cancel == nil && ctx.Done() == nil {
                return
@@ -229,10 +229,10 @@ func (cs *clientStream) awaitRequestCancel(req *http.Request) {
        select {
        case <-req.Cancel:
                cs.cancelStream()
-               cs.bufPipe.CloseWithError(errRequestCanceled)
+               rl.endStreamError(cs, errRequestCanceled)
        case <-ctx.Done():
                cs.cancelStream()
-               cs.bufPipe.CloseWithError(ctx.Err())
+               rl.endStreamError(cs, ctx.Err())
        case <-cs.done:
        }
 }
@@ -243,6 +243,8 @@ func (cs *clientStream) cancelStream() {
        cs.didReset = true
        cs.cc.mu.Unlock()
 
+       cs.cc.forgetStreamID(cs.ID)
+
        if !didReset {
                cs.cc.writeStreamReset(cs.ID, ErrCodeCancel, nil)
        }
@@ -1531,7 +1533,7 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra
        cs.bufPipe = pipe{b: &dataBuffer{expected: res.ContentLength}}
        cs.bytesRemain = res.ContentLength
        res.Body = transportResponseBody{cs}
-       go cs.awaitRequestCancel(cs.req)
+       go cs.awaitRequestCancel(cs.req, rl)
 
        if cs.requestedGzip && res.Header.Get("Content-Encoding") == "gzip" {
                res.Header.Del("Content-Encoding")

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
@ALTree ALTree changed the title [x/net/http2] Cancelling a request from the Client doesn't release its stream. x/net/http2: cancelling a request from the Client doesn't release its stream Jul 31, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 31, 2017
@BenPope

This comment has been minimized.

Copy link
Author

@BenPope BenPope commented Aug 1, 2017

I've just noticed there's a race on clientConnReadLoop.activeRes within my patch, which is unfortunate.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Aug 7, 2017

I think I'm fixing this in https://go-review.googlesource.com/c/53250.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 7, 2017

Change https://golang.org/cl/53250 mentions this issue: http2: block RoundTrip when the Transport hits MaxConcurrentStreams

@BenPope

This comment has been minimized.

Copy link
Author

@BenPope BenPope commented Aug 7, 2017

Yes, I believe so.

gopherbot pushed a commit to golang/net that referenced this issue Aug 9, 2017
Currently if the http2.Transport hits SettingsMaxConcurrentStreams for a
server, it just makes a new TCP connection and creates the stream on the
new connection. This CL updates that behavior to instead block RoundTrip
until a new stream is available.

I also fixed a second bug, which was necessary to make some tests pass:
Previously, a stream was removed from cc.streams only if either (a) we
received END_STREAM from the server, or (b) we received RST_STREAM from
the server. This CL removes a stream from cc.streams if the request was
cancelled (via ctx.Close, req.Cancel, or resp.Body.Close) before
receiving END_STREAM or RST_STREAM from the server.

Updates golang/go#13774
Updates golang/go#20985
Updates golang/go#21229

Change-Id: I660ffd724c4c513e0f1cc587b404bedb8aff80be
Reviewed-on: https://go-review.googlesource.com/53250
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2017

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

@gopherbot gopherbot closed this in 6b6b9f6 Aug 9, 2017
@jsoriano jsoriano mentioned this issue Aug 22, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Currently if the http2.Transport hits SettingsMaxConcurrentStreams for a
server, it just makes a new TCP connection and creates the stream on the
new connection. This CL updates that behavior to instead block RoundTrip
until a new stream is available.

I also fixed a second bug, which was necessary to make some tests pass:
Previously, a stream was removed from cc.streams only if either (a) we
received END_STREAM from the server, or (b) we received RST_STREAM from
the server. This CL removes a stream from cc.streams if the request was
cancelled (via ctx.Close, req.Cancel, or resp.Body.Close) before
receiving END_STREAM or RST_STREAM from the server.

Updates golang/go#13774
Updates golang/go#20985
Updates golang/go#21229

Change-Id: I660ffd724c4c513e0f1cc587b404bedb8aff80be
Reviewed-on: https://go-review.googlesource.com/53250
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Aug 9, 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
4 participants
You can’t perform that action at this time.