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

http: RoundTrippers shouldn't mutate Request

Fixes issue 2146

[bradfitz, 2011-10-13 17:44:59]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-10-13 19:38:50]

Can we make it not actually mutate the Request?
If not, we should explicitly allow it.
Breaking it and putting it back together fails
mysteriously in a concurrent world.

http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/client.go
File src/pkg/http/client.go (right):

http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/client.go#newcode99
src/pkg/http/client.go:99: err = os.NewError("http: no http.Client.Transport or
http.DefaultTransport")
s/http.//g

http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go
File src/pkg/http/transport.go (right):

http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:309: pconn.mutateRequestFunc = func(req *Request)
func(*Request) {
Could you add a comment or two here about what is
going on?  At first glance this looks like it is mutating
the request, which is exactly what the RoundTripper is
not supposed to do.

http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:464: // written.  It returns a function to undo its
mutations after
Oh, I see.  This is actually worse than mutating the request,
because you can't tell that it got changed once the 
function returns.

The failure mode is going to be people making the same
request in multiple goroutines simultaneously and 
wondering why sometimes it works and sometimes they
get weird behavior.

If the RoundTripper isn't supposed to edit the request,
it shouldn't edit the request.

[bradfitz, 2011-10-13 19:53:10]

yeah...

On Thu, Oct 13, 2011 at 12:38 PM, <rsc@golang.org> wrote:

> Can we make it not actually mutate the Request?
> If not, we should explicitly allow it.
> Breaking it and putting it back together fails
> mysteriously in a concurrent world.
>
>
>
>
http://codereview.appspot.com/**5284041/diff/6001/src/pkg/**http/client.go<ht...
> File src/pkg/http/client.go (right):
>
> http://codereview.appspot.com/**5284041/diff/6001/src/pkg/**
>
http/client.go#newcode99<http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/client.go#newcode99>
> src/pkg/http/client.go:99: err = os.NewError("http: no
> http.Client.Transport or http.DefaultTransport")
> s/http.//g
>
> http://codereview.appspot.com/**5284041/diff/6001/src/pkg/**
>
http/transport.go<http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go>
> File src/pkg/http/transport.go (right):
>
> http://codereview.appspot.com/**5284041/diff/6001/src/pkg/**
>
http/transport.go#newcode309<http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go#newcode309>
> src/pkg/http/transport.go:309: pconn.mutateRequestFunc = func(req
> *Request) func(*Request) {
> Could you add a comment or two here about what is
> going on?  At first glance this looks like it is mutating
> the request, which is exactly what the RoundTripper is
> not supposed to do.
>
> http://codereview.appspot.com/**5284041/diff/6001/src/pkg/**
>
http/transport.go#newcode464<http://codereview.appspot.com/5284041/diff/6001/src/pkg/http/transport.go#newcode464>
> src/pkg/http/transport.go:464: // written.  It returns a function to
> undo its mutations after
> Oh, I see.  This is actually worse than mutating the request,
> because you can't tell that it got changed once the
> function returns.
>
> The failure mode is going to be people making the same
> request in multiple goroutines simultaneously and
> wondering why sometimes it works and sometimes they
> get weird behavior.
>
> If the RoundTripper isn't supposed to edit the request,
> it shouldn't edit the request.
>
>
http://codereview.appspot.com/**5284041/<http://codereview.appspot.com/5284041/>
>

[bradfitz, 2011-10-13 20:36:57]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-10-13 20:38:14]

PTAL.

No test changes were required, but since I hit a (useful) test failure
during development, I cleaned up one of the tests in the process.

On Thu, Oct 13, 2011 at 1:36 PM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5284041/<http://codereview.appspot.com/5284041/>
>

[rsc, 2011-10-14 16:44:45]

Two questions.

1. What parts of the request actually need to be modified?
Can we write the code so that they don't need to be modified?

2. This looks like a royal pain.  Why are we not instead
just documenting that the RoundTripper can edit the request?

[bradfitz, 2011-10-14 16:49:52]

On Fri, Oct 14, 2011 at 9:44 AM, Russ Cox <rsc@golang.org> wrote:

> Two questions.
>
> 1. What parts of the request actually need to be modified?
>

a) Proxy-Authorization header
b) Accept-Encoding: gzip header


> Can we write the code so that they don't need to be modified?
>

We'd need a way to write a request + body with extra headers, but adding
such a function is still several hops away from a
ClientConn.Write(*Request).


> 2. This looks like a royal pain.


yes.  but mostly the cloning code, but I think that could be useful
generally and should be exported at some point.

I think this CL is much simpler than the previous version.


> Why are we not instead
> just documenting that the RoundTripper can edit the request?
>

That's how it was documented, but people complained (leading to earlier
fixes cleaning up after gzip header), and logically it really shouldn't
mutate the request.  You originally made the request that this shouldn't
mutate the request, hence my filing the bug.

[rsc, 2011-10-14 17:06:03]

If we're just talking about two headers, how bad is it to just
have a Request.Write that takes care of those two headers 
specially?  We already have variants that filter out certain
headers, so it seems like we're pretty close.

I'm of two minds about being able to mutate the request.
On the one hand, it *does* mutate the Body (by reading it).
On the other hand, a large number of important requests
do not have bodies.

Russ

[bradfitz, 2011-10-14 19:49:53]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-10-14 19:58:31]

On Fri, Oct 14, 2011 at 10:05 AM, Russ Cox <rsc@golang.org> wrote:

> If we're just talking about two headers, how bad is it to just
> have a Request.Write that takes care of those two headers
> specially?  We already have variants that filter out certain
> headers, so it seems like we're pretty close.
>

true.

Done.  PTAL.

I'm of two minds about being able to mutate the request.
> On the one hand, it *does* mutate the Body (by reading it).
>

Want me to clarify that Body may be consumed?


> On the other hand, a large number of important requests
> do not have bodies.
>
> Russ
>

[rsc, 2011-10-14 21:09:43]

LGTM

Up to you whether to mention Body.

[bradfitz, 2011-10-14 21:16:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=18eaae34f73e ***

http: RoundTrippers shouldn't mutate Request

Fixes issue 2146

R=rsc
CC=golang-dev
http://codereview.appspot.com/5284041