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

Closed
woodliu opened this issue Sep 22, 2019 · 6 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@woodliu
Copy link

woodliu 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
Copy link
Contributor

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

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 23, 2019
@woodliu
Copy link
Author

woodliu 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 Issue is not actionable because of missing required information, which needs to be provided. label Sep 23, 2019
@bcmills
Copy link
Contributor

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

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 23, 2019
@woodliu
Copy link
Author

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

@mvdan
Copy link
Member

mvdan commented Jun 15, 2021

Closing old issues that still have the WaitingForInfo label where enough details to investigate weren't provided. Feel free to leave a comment with more details and we can reopen.

@mvdan mvdan closed this as completed Jun 15, 2021
@woodliu
Copy link
Author

woodliu commented Nov 1, 2021

@mvdan Sorry about the long time to reply. I review the code again, it doesn't need to close the conn even when roundTrip return err. Because the connection may broken by a short time accident(may cause by reboot a gateway), and it can be used soon .In this case, we can use it again instead recreate a new connection(especially when a connection switch between connect and disconnect state). when roundTrip return err, it should keep available until timeout.
So this is not a issue.

@golang golang locked and limited conversation to collaborators Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants