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: Transport should always verify we wrote the request #7569

Closed
bradfitz opened this issue Mar 17, 2014 · 2 comments
Closed

net/http: Transport should always verify we wrote the request #7569

bradfitz opened this issue Mar 17, 2014 · 2 comments
Milestone

Comments

@bradfitz
Copy link
Contributor

While debugging something else, I noticed:

https://golang.org/issue/7521?c=2

in transport.go's

func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err error) {

... we only check the return value of Request.Write in the case where it happens before
the server replies. If the server writes half of it, pauses for a moment, and then the
server replies with a valid happy response, we respond successfully, even if the write
subsequently then fails.

Bigger fear: could we then mark a persistConn as idle before it's finished writing and
enqueue a second write behind it, even if the first one is blocked or will fail?
@gopherbot
Copy link

Comment 1:

CL https://golang.org/cl/86290043 mentions this issue.

@bradfitz
Copy link
Contributor Author

Comment 2:

*** Submitted as https://code.google.com/p/go/source/detail?r=07e31caba5b6 ***
net/http: don't reuse Transport connection unless Request.Write finished
In a typical HTTP request, the client writes the request, and
then the server replies. Go's HTTP client code (Transport) has
two goroutines per connection: one writing, and one reading. A
third goroutine (the one initiating the HTTP request)
coordinates with those two.
Because most HTTP requests are done when the server replies,
the Go code has always handled connection reuse purely in the
readLoop goroutine.
But if a client is writing a large request and the server
replies before it's consumed the entire request (e.g. it
replied with a 403 Forbidden and had no use for the body), it
was possible for Go to re-select that connection for a
subsequent request before we were done writing the first. That
wasn't actually a data race; the second HTTP request would
just get enqueued to write its request on the writeLoop. But
because the previous writeLoop didn't finish writing (and
might not ever), that connection is in a weird state. We
really just don't want to get into a state where we're
re-using a connection when the server spoke out of turn.
This CL changes the readLoop goroutine to verify that the
writeLoop finished before returning the connection.
In the process, it also fixes a potential goroutine leak where
a connection could close but the recycling logic could be
blocked forever waiting for the client to read to EOF or
error. Now it also selects on the persistConn's close channel,
and the closer of that is no longer the readLoop (which was
dead locking in some cases before). It's now closed at the
same place the underlying net.Conn is closed. This likely fixes
or helps issue #7620.
Also addressed some small cosmetic things in the process.
Update issue #7620
Fixes issue #7569

Status changed to Fixed.

@bradfitz bradfitz self-assigned this Apr 10, 2014
@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants