code review 76800043: net/http/fcgi: fix handling of request ID reuse

net/http/fcgi: fix handling of request ID reuse

Request ID reuse is allowed by the FastCGI spec [1]. In particular nginx uses
the same request ID, 1, for all requests on a given connection. Because
serveRequest does not remove the request from conn.requests, this causes it to
treat the second request as a duplicate and drops the connection immediately
after beginRequest. This manifests with nginx option 'fastcgi_keep_conn on' as
the following message in nginx error log:

2014/03/17 01:39:13 [error] 730#0: *109 recv() failed (104: Connection reset by peer) while reading response header from upstream, client: x.x.x.x, server: example.org, request: "GET / HTTP/1.1", upstream: "fastcgi://127.0.0.1:9001", host: "example.org"

Because handleRecord and serveRequest run in different goroutines, access to
conn.requests must now be synchronized.

[1] http://www.fastcgi.com/drupal/node/6?q=node/22#S3.3

[catalinp, 2014-03-17 16:40:53]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-03-17 19:02:32]

Makes sense.

Little style nit:

https://codereview.appspot.com/76800043/diff/40001/src/pkg/net/http/fcgi/chil...
File src/pkg/net/http/fcgi/child.go (right):

https://codereview.appspot.com/76800043/diff/40001/src/pkg/net/http/fcgi/chil...
src/pkg/net/http/fcgi/child.go:130: mutex    sync.Mutex // protects requests
Name this "mu" and put it in its own block above what it guards (still with the
comment)

so:

type child struct {
   conn     *conn
   handler  http.Handler

   mu       sync.Mutex // guards requests:
   requests map....
}

[catalinp, 2014-03-17 20:21:46]

https://codereview.appspot.com/76800043/diff/40001/src/pkg/net/http/fcgi/chil...
File src/pkg/net/http/fcgi/child.go (right):

https://codereview.appspot.com/76800043/diff/40001/src/pkg/net/http/fcgi/chil...
src/pkg/net/http/fcgi/child.go:130: mutex    sync.Mutex // protects requests
On 2014/03/17 19:02:32, bradfitz wrote:
> Name this "mu" and put it in its own block above what it guards (still with
the
> comment)
> 
> so:
> 
> type child struct {
>    conn     *conn
>    handler  http.Handler
> 
>    mu       sync.Mutex // guards requests:
>    requests map....
> }

Done.

I was following the style of 'type conn':
http://golang.org/src/pkg/net/http/fcgi/fcgi.go#L101

[bradfitz, 2014-03-17 22:47:14]

LGTM



On Mon, Mar 17, 2014 at 1:21 PM, <catalinp@google.com> wrote:

>
> https://codereview.appspot.com/76800043/diff/40001/src/
> pkg/net/http/fcgi/child.go
> File src/pkg/net/http/fcgi/child.go (right):
>
> https://codereview.appspot.com/76800043/diff/40001/src/
> pkg/net/http/fcgi/child.go#newcode130
> src/pkg/net/http/fcgi/child.go:130: mutex    sync.Mutex // protects
> requests
> On 2014/03/17 19:02:32, bradfitz wrote:
>
>> Name this "mu" and put it in its own block above what it guards (still
>>
> with the
>
>> comment)
>>
>
>  so:
>>
>
>  type child struct {
>>     conn     *conn
>>     handler  http.Handler
>>
>
>      mu       sync.Mutex // guards requests:
>>     requests map....
>> }
>>
>
> Done.
>
> I was following the style of 'type conn':
> http://golang.org/src/pkg/net/http/fcgi/fcgi.go#L101
>
> https://codereview.appspot.com/76800043/
>

[bradfitz, 2014-03-17 22:47:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=c69077fe03a5 ***

net/http/fcgi: fix handling of request ID reuse

Request ID reuse is allowed by the FastCGI spec [1]. In particular nginx uses
the same request ID, 1, for all requests on a given connection. Because
serveRequest does not remove the request from conn.requests, this causes it to
treat the second request as a duplicate and drops the connection immediately
after beginRequest. This manifests with nginx option 'fastcgi_keep_conn on' as
the following message in nginx error log:

2014/03/17 01:39:13 [error] 730#0: *109 recv() failed (104: Connection reset by
peer) while reading response header from upstream, client: x.x.x.x, server:
example.org, request: "GET / HTTP/1.1", upstream: "fastcgi://127.0.0.1:9001",
host: "example.org"

Because handleRecord and serveRequest run in different goroutines, access to
conn.requests must now be synchronized.

[1] http://www.fastcgi.com/drupal/node/6?q=node/22#S3.3

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/76800043

Committer: Brad Fitzpatrick <bradfitz@golang.org>