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://golang.org/cl/76800043

diff --git a/src/pkg/net/http/fcgi/child.go b/src/pkg/net/http/fcgi/child.go
index 60b794e077506e73c89f671c9a41697ffafa9dbc..a3beaa33a8604cb35a7258078ef546021438cec6 100644 (file)
--- a/src/pkg/net/http/fcgi/child.go
+++ b/src/pkg/net/http/fcgi/child.go
@@ -16,6 +16,7 @@ import (
        "net/http/cgi"
        "os"
        "strings"
+       "sync"
        "time"
 )
 
@@ -126,8 +127,10 @@ func (r *response) Close() error {
 }
 
 type child struct {
-       conn     *conn
-       handler  http.Handler
+       conn    *conn
+       handler http.Handler
+
+       mu       sync.Mutex          // protects requests:
        requests map[uint16]*request // keyed by request ID
 }
 
@@ -157,7 +160,9 @@ var errCloseConn = errors.New("fcgi: connection should be closed")
 var emptyBody = ioutil.NopCloser(strings.NewReader(""))
 
 func (c *child) handleRecord(rec *record) error {
+       c.mu.Lock()
        req, ok := c.requests[rec.h.Id]
+       c.mu.Unlock()
        if !ok && rec.h.Type != typeBeginRequest && rec.h.Type != typeGetValues {
                // The spec says to ignore unknown request IDs.
                return nil
@@ -179,7 +184,10 @@ func (c *child) handleRecord(rec *record) error {
                        c.conn.writeEndRequest(rec.h.Id, 0, statusUnknownRole)
                        return nil
                }
-               c.requests[rec.h.Id] = newRequest(rec.h.Id, br.flags)
+               req = newRequest(rec.h.Id, br.flags)
+               c.mu.Lock()
+               c.requests[rec.h.Id] = req
+               c.mu.Unlock()
                return nil
        case typeParams:
                // NOTE(eds): Technically a key-value pair can straddle the boundary
@@ -220,7 +228,9 @@ func (c *child) handleRecord(rec *record) error {
                return nil
        case typeAbortRequest:
                println("abort")
+               c.mu.Lock()
                delete(c.requests, rec.h.Id)
+               c.mu.Unlock()
                c.conn.writeEndRequest(rec.h.Id, 0, statusRequestComplete)
                if !req.keepConn {
                        // connection will close upon return
@@ -247,6 +257,9 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
                c.handler.ServeHTTP(r, httpReq)
        }
        r.Close()
+       c.mu.Lock()
+       delete(c.requests, req.reqId)
+       c.mu.Unlock()
        c.conn.writeEndRequest(req.reqId, 0, statusRequestComplete)
 
        // Consume the entire body, so the host isn't still writing to