code review 5284041: http: RoundTrippers shouldn't mutate Request

src/pkg/http/transport.go

 // Copyright 2011 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 package http
 
 import (
         "bufio"
         "compress/gzip"
         "crypto/tls"
@@ skipping 82 matching lines @@
 }
 
 // ProxyURL returns a proxy function (for use in a Transport)
 // that always returns the same URL.
 func ProxyURL(fixedURL *url.URL) func(*Request) (*url.URL, os.Error) {
         return func(*Request) (*url.URL, os.Error) {
                 return fixedURL, nil
         }
 }
 
+// transportRequest is a wrapper around a *Request that adds
+// optional extra headers to write.
+type transportRequest struct {
+        *Request        // original request, not to be mutated
+        extra    Header // extra headers to write, or nil
+}
+
+func (tr *transportRequest) extraHeaders() Header {
+        if tr.extra == nil {
+                tr.extra = make(Header)
+        }
+        return tr.extra
+}
+
 // RoundTrip implements the RoundTripper interface.
 func (t *Transport) RoundTrip(req *Request) (resp *Response, err os.Error) {
         if req.URL == nil {
                 return nil, os.NewError("http: nil Request.URL")
+        }
+        if req.Header == nil {
+                return nil, os.NewError("http: nil Request.Header")
         }
         if req.URL.Scheme != "http" && req.URL.Scheme != "https" {
                 t.lk.Lock()
                 var rt RoundTripper
                 if t.altProto != nil {
                         rt = t.altProto[req.URL.Scheme]
                 }
                 t.lk.Unlock()
                 if rt == nil {
                         return nil, &badStringError{"unsupported protocol scheme", req.URL.Scheme}
                 }
                 return rt.RoundTrip(req)
         }
-
-        cm, err := t.connectMethodForRequest(req)
+        treq := &transportRequest{Request: req}
+        cm, err := t.connectMethodForRequest(treq)
         if err != nil {
                 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(cm)
         if err != nil {
                 return nil, err
         }
 
-        return pconn.roundTrip(req)
+        return pconn.roundTrip(treq)
 }
 
 // RegisterProtocol registers a new protocol with scheme.
 // The Transport will pass requests using the given scheme to rt.
 // It is rt's responsibility to simulate HTTP request semantics.
 //
 // RegisterProtocol can be used by other packages to provide
 // implementations of protocol schemes like "ftp" or "file".
 func (t *Transport) RegisterProtocol(scheme string, rt RoundTripper) {
         if scheme == "http" || scheme == "https" {
@@ skipping 32 matching lines @@
 // Private implementation past this point.
 //
 
 func getenvEitherCase(k string) string {
         if v := os.Getenv(strings.ToUpper(k)); v != "" {
                 return v
         }
         return os.Getenv(strings.ToLower(k))
 }
 
-func (t *Transport) connectMethodForRequest(req *Request) (*connectMethod, os.Error) {
+func (t *Transport) connectMethodForRequest(treq *transportRequest) (*connectMethod, os.Error) {
         cm := &connectMethod{
-                targetScheme: req.URL.Scheme,
-                targetAddr:   canonicalAddr(req.URL),
+                targetScheme: treq.URL.Scheme,
+                targetAddr:   canonicalAddr(treq.URL),
         }
         if t.Proxy != nil {
                 var err os.Error
-                cm.proxyURL, err = t.Proxy(req)
+                cm.proxyURL, err = t.Proxy(treq.Request)
                 if err != nil {
                         return nil, err
                 }
         }
         return cm, nil
 }
 
 // proxyAuth returns the Proxy-Authorization header to set
 // on requests, if applicable.
 func (cm *connectMethod) proxyAuth() string {
@@ skipping 82 matching lines @@
         }
 
         pa := cm.proxyAuth()
 
         pconn := &persistConn{
                 t:        t,
                 cacheKey: cm.String(),
                 conn:     conn,
                 reqch:    make(chan requestAndChan, 50),
         }
-        newClientConnFunc := NewClientConn
 
         switch {
         case cm.proxyURL == nil:
                 // Do nothing.
         case cm.targetScheme == "http":
-                newClientConnFunc = NewProxyClientConn
+                pconn.isProxy = true
                 if pa != "" {
-                        pconn.mutateRequestFunc = func(req *Request) {
-                                if req.Header == nil {
-                                        req.Header = make(Header)
-                                }
-                                req.Header.Set("Proxy-Authorization", pa)
+                        pconn.mutateHeaderFunc = func(h Header) {
+                                h.Set("Proxy-Authorization", pa)
                         }
                 }
         case cm.targetScheme == "https":
                 connectReq := &Request{
                         Method: "CONNECT",
                         URL:    &url.URL{RawPath: cm.targetAddr},
                         Host:   cm.targetAddr,
                         Header: make(Header),
                 }
                 if pa != "" {
@@ skipping 23 matching lines @@
                 if err = conn.(*tls.Conn).Handshake(); err != nil {
                         return nil, err
                 }
                 if err = conn.(*tls.Conn).VerifyHostname(cm.tlsHost()); err != nil {
                         return nil, err
                 }
                 pconn.conn = conn
         }
 
         pconn.br = bufio.NewReader(pconn.conn)
-        pconn.cc = newClientConnFunc(conn, pconn.br)
+        pconn.cc = NewClientConn(conn, pconn.br)
         go pconn.readLoop()
         return pconn, nil
 }
 
 // useProxy returns true if requests to addr should use a proxy,
 // according to the NO_PROXY or no_proxy environment variable.
 // addr is always a canonicalAddr with a host and port.
 func useProxy(addr string) bool {
         if len(addr) == 0 {
                 return true
@@ skipping 75 matching lines @@
 // tlsHost returns the host name to match against the peer's
 // TLS certificate.
 func (cm *connectMethod) tlsHost() string {
         h := cm.targetAddr
         if hasPort(h) {
                 h = h[:strings.LastIndex(h, ":")]
         }
         return h
 }
 
-type readResult struct {
-        res *Response // either res or err will be set
-        err os.Error
-}
-
-type writeRequest struct {
-        // Set by client (in pc.roundTrip)
-        req   *Request
-        resch chan *readResult
-
-        // Set by writeLoop if an error writing headers.
-        writeErr os.Error
-}
-
 // persistConn wraps a connection, usually a persistent one
 // (but may be used for non-keep-alive requests as well)
 type persistConn struct {
-        t                 *Transport
-        cacheKey          string // its connectMethod.String()
-        conn              net.Conn
-        cc                *ClientConn
-        br                *bufio.Reader
-        reqch             chan requestAndChan // written by roundTrip(); read by readLoop()
-        mutateRequestFunc func(*Request)      // nil or func to modify each outbound request
+        t        *Transport
+        cacheKey string // its connectMethod.String()
+        conn     net.Conn
+        cc       *ClientConn
+        br       *bufio.Reader
+        reqch    chan requestAndChan // written by roundTrip(); read by readLoop()
+        isProxy  bool
+
+        // mutateHeaderFunc is an optional func to modify extra
+        // headers on each outbound request before it's written. (the
+        // original Request given to RoundTrip is not modified)
+        mutateHeaderFunc func(Header)
 
         lk                   sync.Mutex // guards numExpectedResponses and broken
         numExpectedResponses int
         broken               bool // an error has happened on this connection; marked broken so it's not reused.
 }
 
 func (pc *persistConn) isBroken() bool {
         pc.lk.Lock()
         defer pc.lk.Unlock()
         return pc.broken
@@ skipping 34 matching lines @@
                                 string(pb), err)
                         pc.close()
                         return
                 }
 
                 rc := <-pc.reqch
                 resp, err := pc.cc.readUsing(rc.req, func(buf *bufio.Reader, forReq *Request) (*Response, os.Error) {
                         resp, err := ReadResponse(buf, forReq)
                         if err != nil || resp.ContentLength == 0 {
                                 return resp, err
-                        }
-                        if rc.addedGzip {
-                                forReq.Header.Del("Accept-Encoding")
                         }
                         if rc.addedGzip && resp.Header.Get("Content-Encoding") == "gzip" {
                                 resp.Header.Del("Content-Encoding")
                                 resp.Header.Del("Content-Length")
                                 resp.ContentLength = -1
                                 gzReader, err := gzip.NewReader(resp.Body)
                                 if err != nil {
                                         pc.close()
                                         return nil, err
                                 }
@@ skipping 56 matching lines @@
 type requestAndChan struct {
         req *Request
         ch  chan responseAndError
 
         // did the Transport (as opposed to the client code) add an
         // Accept-Encoding gzip header? only if it we set it do
         // we transparently decode the gzip.
         addedGzip bool
 }
 
-func (pc *persistConn) roundTrip(req *Request) (resp *Response, err os.Error) {
-        if pc.mutateRequestFunc != nil {
-                pc.mutateRequestFunc(req)
+func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err os.Error) {
+        if pc.mutateHeaderFunc != nil {
+                pc.mutateHeaderFunc(req.extraHeaders())
         }
 
         // Ask for a compressed version if the caller didn't set their
         // own value for Accept-Encoding. We only attempted to
         // uncompress the gzip stream if we were the layer that
         // requested it.
         requestedGzip := false
         if !pc.t.DisableCompression && req.Header.Get("Accept-Encoding") == "" {
                 // Request gzip only, not deflate. Deflate is ambiguous and·
-                // as universally supported anyway.
+                // not as universally supported anyway.
                 // See: http://www.gzip.org/zlib/zlib_faq.html#faq38
                 requestedGzip = true
-                req.Header.Set("Accept-Encoding", "gzip")
+                req.extraHeaders().Set("Accept-Encoding", "gzip")
         }
 
         pc.lk.Lock()
         pc.numExpectedResponses++
         pc.lk.Unlock()
 
-        err = pc.cc.Write(req)
+        pc.cc.writeReq = func(r *Request, w io.Writer) os.Error {
+                return r.write(w, pc.isProxy, req.extra)
+        }
+
+        err = pc.cc.Write(req.Request)
         if err != nil {
                 pc.close()
                 return
         }
 
         ch := make(chan responseAndError, 1)
-        pc.reqch <- requestAndChan{req, ch, requestedGzip}
+        pc.reqch <- requestAndChan{req.Request, ch, requestedGzip}
         re := <-ch
         pc.lk.Lock()
         pc.numExpectedResponses--
         pc.lk.Unlock()
 
         return re.res, re.err
 }
 
 func (pc *persistConn) close() {
         pc.lk.Lock()
         defer pc.lk.Unlock()
         pc.broken = true
         pc.cc.Close()
         pc.conn.Close()
-        pc.mutateRequestFunc = nil
+        pc.mutateHeaderFunc = nil
 }
 
 var portMap = map[string]string{
         "http":  "80",
         "https": "443",
 }
 
 // canonicalAddr returns url.Host but always with a ":port" suffix
 func canonicalAddr(url *url.URL) string {
         addr := url.Host
@@ skipping 60 matching lines @@
 
 // discardOnCloseReadCloser consumes all its input on Close.
 type discardOnCloseReadCloser struct {
         io.ReadCloser
 }
 
 func (d *discardOnCloseReadCloser) Close() os.Error {
         io.Copy(ioutil.Discard, d.ReadCloser) // ignore errors; likely invalid or already closed
         return d.ReadCloser.Close()
 }