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

net/http: shouldRetryRequest won't send a failed request on old connection #34455

Open
woodliu opened this issue Sep 22, 2019 · 4 comments

Comments

@woodliu
Copy link

commented Sep 22, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.9 linux/amd64

Does this issue reproduce with the latest release?

Code review

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build627673678=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Code review

What did you expect to see?

In file net/http/transport.go func (t *Transport) roundTrip(req *Request) (*Response, error)
After pconn.alt.RoundTrip(req) return err not nil, it will call pconn.shouldRetryRequest to see whether we should retry sending a failed HTTP request on a new connection.
But whether shouldRetryRequest return true or false, it won't use the old connection(It will call getConn to get a new idle connection). And the old connection(pconn) still there without closing.

	for {
		select {
		case <-ctx.Done():
			req.closeBody()
			return nil, ctx.Err()
		default:
		}

		// treq gets modified by roundTrip, so we need to recreate for each retry.
		treq := &transportRequest{Request: req, trace: trace}
		cm, err := t.connectMethodForRequest(treq)
		if err != nil {
			req.closeBody()
			return nil, err
		}

		// Get the cached or newly-created connection to either the
		// host (for http or https), the http proxy, or the http proxy
		// pre-CONNECTed to https server. In any case, we'll be ready
		// to send it requests.
		pconn, err := t.getConn(treq, cm)
		if err != nil {
			t.setReqCanceler(req, nil)
			req.closeBody()
			return nil, err
		}

		var resp *Response
		if pconn.alt != nil {
			// HTTP/2 path.
			t.decHostConnCount(cm.key()) // don't count cached http2 conns toward conns per host
			t.setReqCanceler(req, nil)   // not cancelable with CancelRequest
			resp, err = pconn.alt.RoundTrip(req)
		} else {
			time.Sleep(time.Second*2)
			resp, err = pconn.roundTrip(treq)
		}

		if err == nil {
			return resp, nil
		}
		if !pconn.shouldRetryRequest(req, err) {
			// Issue 16465: return underlying net.Conn.Read error from peek,
			// as we've historically done.
			if e, ok := err.(transportReadFromServerError); ok {
				err = e.err
			}
			return nil, err
		}
		testHookRoundTripRetried()

		// Rewind the body if we're able to.
		if req.GetBody != nil {
			newReq := *req
			var err error
			newReq.Body, err = req.GetBody()
			if err != nil {
				return nil, err
			}
			req = &newReq
		}
	}

What did you see instead?

I think transport should :

  • Close the pconn when roundtrip return err
  • Give the old connection a try(I'm not sure if this will cause a dead loop if the old connection return err all the way)
@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

go version go1.12.9 linux/amd64

There were substantial changes to net/http's connection-reuse logic in 1.13. Is this still an issue in the 1.13 release?

@woodliu

This comment has been minimized.

Copy link
Author

commented Sep 23, 2019

After reading the 1.13 code,i think it's the same logic, except that 1.13 add function for http2.

@bcmills bcmills removed the WaitingForInfo label Sep 23, 2019
@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

Can you provide a concrete program that demonstrates the problem, perhaps using net/http/httptrace? It's hard to tell whether something suspicious in a source file is a problem in practice — and particularly hard to tell whether a fix is successful — without a real input that triggers it.

@woodliu

This comment has been minimized.

Copy link
Author

commented Sep 23, 2019

Ok,i will write a script to test it,it may help to find whether it will leak unused persist connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.