code review 6483061: net/http/httputil: fix race in DumpRequestOut

net/http/httputil: fix race in DumpRequestOut

Fixes issue 3892.

Swapping the order of the writers inside the MultiWriter ensures
the request will be written to buf before http.ReadRequest completes.

The fencedBuffer is not required to make the test pass on
any machine that I have access too, but as the buf is shared
across goroutines, I think it is necessary for correctness.

[dave_cheney.net, 2012-08-28 07:08:23]

Hello bradfitz@golang.org, fullung@gmail.com (cc: golang-dev@googlegroups.com),

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

[fss, 2012-08-28 11:01:48]

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
File src/pkg/net/http/httputil/dump.go (right):

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
src/pkg/net/http/httputil/dump.go:116: return fb.Buffer.Write(b)
Why not just `fb.Lock`, `fb.Unlock` and `fb.Write`?

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
src/pkg/net/http/httputil/dump.go:122: return fb.Buffer.Bytes()
ditto

[dave_cheney.net, 2012-08-28 11:06:03]

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
File src/pkg/net/http/httputil/dump.go (right):

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
src/pkg/net/http/httputil/dump.go:116: return fb.Buffer.Write(b)
On 2012/08/28 11:01:48, fss wrote:
> Why not just `fb.Lock`, `fb.Unlock` and `fb.Write`?

It wouldn't be much of a mutex if it didn't protect the buffer's len and cap
values. fencedBuffer is probably the wrong name, syncBuffer might be a better
choice.

[fss, 2012-08-28 13:09:01]

On 2012/08/28 11:06:03, dfc wrote:
>
http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
> File src/pkg/net/http/httputil/dump.go (right):
> 
>
http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
> src/pkg/net/http/httputil/dump.go:116: return fb.Buffer.Write(b)
> On 2012/08/28 11:01:48, fss wrote:
> > Why not just `fb.Lock`, `fb.Unlock` and `fb.Write`?
> 
> It wouldn't be much of a mutex if it didn't protect the buffer's len and cap
> values. fencedBuffer is probably the wrong name, syncBuffer might be a better
> choice.

I see, my suggestion is about the syntax only: you could drop Buffer and Mutex,
and use just fb.Write, fb.Bytes, fb.Lock, fb.Unlock, etc.

[dave_cheney.net, 2012-08-28 13:18:09]

> I see, my suggestion is about the syntax only: you could drop Buffer and
Mutex,
> and use just fb.Write, fb.Bytes, fb.Lock, fb.Unlock, etc.

But then fb.Write would forward to fb.Buffer.Write without holding the lock.
Could you please rewrite the methods in your comment to show me what you mean.

[fss, 2012-08-28 13:24:33]

On 2012/08/28 13:18:09, dfc wrote:
> [cut]
> But then fb.Write would forward to fb.Buffer.Write without holding the lock.
> Could you please rewrite the methods in your comment to show me what you mean.

Sorry for the miscommunication. Now I included the code, that can't be
miscommunicated :)

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
File src/pkg/net/http/httputil/dump.go (right):

http://codereview.appspot.com/6483061/diff/10001/src/pkg/net/http/httputil/du...
src/pkg/net/http/httputil/dump.go:116: return fb.Buffer.Write(b)
func (fb *fencedBuffer) Write(b []byte) (int, error) {
    fb.Lock()
    defer fb.Unlock()
    return fb.Write(b)
}

func (fb *fencedBuffer) Bytes() []byte {
    fb.Lock()
    defer db.Unlock()
    return fb.Bytes()
}

[dave_cheney.net, 2012-08-28 13:26:25]

> func (fb *fencedBuffer) Write(b []byte) (int, error) {
>     fb.Lock()
>     defer fb.Unlock()
>     return fb.Write(b)
> }

Right, you are correct that I can call the embedded Lock/Unlock
directly. Sorry for being dense

however return fb.Write(b) will cause infinite recursion.

>
> func (fb *fencedBuffer) Bytes() []byte {
>     fb.Lock()
>     defer db.Unlock()
>     return fb.Bytes()
> }
>
> http://codereview.appspot.com/6483061/

ditto.

[fss, 2012-08-28 13:29:11]

On 2012/08/28 13:26:25, dfc wrote:
> > func (fb *fencedBuffer) Write(b []byte) (int, error) {
> >     fb.Lock()
> >     defer fb.Unlock()
> >     return fb.Write(b)
> > }
> 
> Right, you are correct that I can call the embedded Lock/Unlock
> directly. Sorry for being dense
> 
> however return fb.Write(b) will cause infinite recursion.
> 

ouch. Nevermind, sorry for the noise.

[dave_cheney.net, 2012-08-28 13:30:23]

Hello bradfitz@golang.org, fullung@gmail.com, franciscossouza@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-08-28 14:07:36]

LGTM but I'd rather drop the syncBuffer wrapper if it's not required.

It's more code and more complexity, and the comment on it doesn't really justify
its existence.  "published correctly" doesn't say much to me.

But I defer to others.

Actually, the more I think about it, if we're writing to a bytes.Buffer now
before the PipeWriter (which I agree with), the PipeWriter/PipeReader have
synchronization.  And we read from the PipeReader before accessing buf.Bytes()
anyway, right?

I'd just make this CL be about the order flip for now.

https://codereview.appspot.com/6483061/diff/11002/src/pkg/net/http/httputil/d...
File src/pkg/net/http/httputil/dump.go (right):

https://codereview.appspot.com/6483061/diff/11002/src/pkg/net/http/httputil/d...
src/pkg/net/http/httputil/dump.go:107: // are published correctly
end in a period.

[dave_cheney.net, 2012-08-28 14:10:24]

SGTM. I'll revert the syncBuffer change until it proves necessary. 

On 29/08/2012, at 0:07, bradfitz@golang.org wrote:

> LGTM but I'd rather drop the syncBuffer wrapper if it's not required.
> 
> It's more code and more complexity, and the comment on it doesn't really
> justify its existence.  "published correctly" doesn't say much to me.
> 
> But I defer to others.
> 
> Actually, the more I think about it, if we're writing to a bytes.Buffer
> now before the PipeWriter (which I agree with), the
> PipeWriter/PipeReader have synchronization.  And we read from the
> PipeReader before accessing buf.Bytes() anyway, right?
> 
> I'd just make this CL be about the order flip for now.
> 
> 
> 
>
https://codereview.appspot.com/6483061/diff/11002/src/pkg/net/http/httputil/d...
> File src/pkg/net/http/httputil/dump.go (right):
> 
>
https://codereview.appspot.com/6483061/diff/11002/src/pkg/net/http/httputil/d...
> src/pkg/net/http/httputil/dump.go:107: // are published correctly
> end in a period.
> 
> https://codereview.appspot.com/6483061/

[dave_cheney.net, 2012-08-28 22:54:58]

Hello bradfitz@golang.org, fullung@gmail.com, franciscossouza@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-08-28 22:59:27]

LGTM

On Tue, Aug 28, 2012 at 3:54 PM, <dave@cheney.net> wrote:

> Hello bradfitz@golang.org, fullung@gmail.com, franciscossouza@gmail.com
> (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6483061/<http://codereview.appspot.com/6483061/>
>

[dave_cheney.net, 2012-08-28 23:06:16]

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

net/http/httputil: fix race in DumpRequestOut

Fixes issue 3892.

Swapping the order of the writers inside the MultiWriter ensures
the request will be written to buf before http.ReadRequest completes.

The fencedBuffer is not required to make the test pass on
any machine that I have access too, but as the buf is shared
across goroutines, I think it is necessary for correctness.

R=bradfitz, fullung, franciscossouza
CC=golang-dev
http://codereview.appspot.com/6483061