net/http: Fixed race condition when server closes the connection during setup

net/http: Fixed race condition when server closes the connection during setup.

[bradfitz, 2012-07-20 20:02:29]

This would need a test before being submitted.

But could you explain the race first?


On Fri, Jul 20, 2012 at 11:23 AM, <nightgunner5@llamaslayers.net> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Description:
> net/http: Fixed race condition when server closes the connection during
> setup.
>
> Please review this at
http://codereview.appspot.com/**6430056/<http://codereview.appspot.com/6430056/>
>
> Affected files:
>   M src/pkg/net/http/transport.go
>
>
> Index: src/pkg/net/http/transport.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/transport.**go
> +++ b/src/pkg/net/http/transport.**go
> @@ -530,8 +530,6 @@
>  }
>
>  func (pc *persistConn) readLoop() {
> -       defer close(pc.closech)
> -       defer close(pc.writech)
>         alive := true
>         var lastbody io.ReadCloser // last response body, if any, read on
> this connection
>
> @@ -693,16 +691,20 @@
>
>         pc.lk.Lock()
>         pc.numExpectedResponses++
> -       pc.lk.Unlock()
>
>         // Write the request concurrently with waiting for a response,
>         // in case the server decides to reply before reading our full
>         // request body.
> +       if pc.broken {
> +               pc.lk.Unlock()
> +               return nil, errors.New("net/http: server closed connection
> early")
> +       }
>         writeErrCh := make(chan error, 1)
>         pc.writech <- writeRequest{req, writeErrCh}
>
>         resc := make(chan responseAndError, 1)
>         pc.reqch <- requestAndChan{req.Request, resc, requestedGzip}
> +       pc.lk.Unlock()
>
>         var re responseAndError
>         var pconnDeadCh = pc.closech
> @@ -765,6 +767,8 @@
>         if !pc.closed {
>                 pc.conn.Close()
>                 pc.closed = true
> +               close(pc.writech)
> +               close(pc.reqch)
>         }
>         pc.mutateHeaderFunc = nil
>  }
>
>
>

[Ben L. (Nightgunner5), 2012-07-20 20:06:42]

It's the reason the build with GOMAXPROCS=32 failed (
https://code.google.com/p/go/source/detail?r=61d1d72136f70da62a456f0e985205c1...
)

The test would be net/http's TestStressSurpriseServerCloses with an 
unreasonably high (32 in the testing I did and the build that failed) 
number for GOMAXPROCS.

On Friday, July 20, 2012 3:02:28 PM UTC-5, Brad Fitzpatrick wrote:
>
> This would need a test before being submitted.
>
> But could you explain the race first?
>
>
> On Fri, Jul 20, 2012 at 11:23 AM, <nightgunner5@llamaslayers.net> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Description:
>> net/http: Fixed race condition when server closes the connection during
>> setup.
>>
>> Please review this at
http://codereview.appspot.com/**6430056/<http://codereview.appspot.com/6430056/>
>>
>> Affected files:
>>   M src/pkg/net/http/transport.go
>>
>>
>> Index: src/pkg/net/http/transport.go
>> ==============================**==============================**=======
>> --- a/src/pkg/net/http/transport.**go
>> +++ b/src/pkg/net/http/transport.**go
>> @@ -530,8 +530,6 @@
>>  }
>>
>>  func (pc *persistConn) readLoop() {
>> -       defer close(pc.closech)
>> -       defer close(pc.writech)
>>         alive := true
>>         var lastbody io.ReadCloser // last response body, if any, read on 
>> this connection
>>
>> @@ -693,16 +691,20 @@
>>
>>         pc.lk.Lock()
>>         pc.numExpectedResponses++
>> -       pc.lk.Unlock()
>>
>>         // Write the request concurrently with waiting for a response,
>>         // in case the server decides to reply before reading our full
>>         // request body.
>> +       if pc.broken {
>> +               pc.lk.Unlock()
>> +               return nil, errors.New("net/http: server closed 
>> connection early")
>> +       }
>>         writeErrCh := make(chan error, 1)
>>         pc.writech <- writeRequest{req, writeErrCh}
>>
>>         resc := make(chan responseAndError, 1)
>>         pc.reqch <- requestAndChan{req.Request, resc, requestedGzip}
>> +       pc.lk.Unlock()
>>
>>         var re responseAndError
>>         var pconnDeadCh = pc.closech
>> @@ -765,6 +767,8 @@
>>         if !pc.closed {
>>                 pc.conn.Close()
>>                 pc.closed = true
>> +               close(pc.writech)
>> +               close(pc.reqch)
>>         }
>>         pc.mutateHeaderFunc = nil
>>  }
>>
>>
>>
>

[Ben L. (Nightgunner5), 2012-08-17 03:02:27]

*** Abandoned ***