http/spdy: fix data race in header decompression.

http/spdy: fix data race in header decompression.

flate's reader greedily reads from the shared io.Reader in Framer. This leads to a data race on Framer.r. Fix this by providing a corkedReader to zlib.NewReaderDict(). We uncork the reader and allow it to read the number of bytes in the compressed payload.

Fixes issue 1884.

[willchan, 2011-05-31 13:13:36]

I think this should fix it (I've tested with GOMAXPROCS=4). Question though, I'm
not explicitly closing the channel in corkedReader. Will it get closed
automagically when Framer gets garbage collected?

[rsc, 2011-05-31 14:54:55]

> I think this should fix it (I've tested with GOMAXPROCS=4). Question
> though, I'm not explicitly closing the channel in corkedReader. Will it
> get closed automagically when Framer gets garbage collected?

Channels don't have to be closed.  You can close a channel
if you want to signal to the reader that the data is over, but
it is equally fine just to let go of all the references and let it
get garbage collected.

[willchan, 2011-05-31 15:00:33]

On Tue, May 31, 2011 at 2:54 PM, Russ Cox <rsc@golang.org> wrote:
>> I think this should fix it (I've tested with GOMAXPROCS=4). Question
>> though, I'm not explicitly closing the channel in corkedReader. Will it
>> get closed automagically when Framer gets garbage collected?
>
> Channels don't have to be closed.  You can close a channel
> if you want to signal to the reader that the data is over, but
> it is equally fine just to let go of all the references and let it
> get garbage collected.
>

Well, what I want to know is that the reader isn't getting leaked.
That even though I'm not explicitly closing the channel, the goroutine
reading from the reader will eventually get an os.EOF and clean up. I
assume this is the case, but I wanted to double check.

[rsc, 2011-05-31 15:01:49]

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go#n...
src/pkg/http/spdy/framer.go:50: bytesAvailable int
I suggest

n int
open bool

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go#n...
src/pkg/http/spdy/framer.go:55: if cr.bytesAvailable == 0 && !cr.eof {
Various comments.  If you invert the bool as above,
you can use it directly in the receive.  Read and ReadFull
are different operations; Read should in general not be
implemented by calling ReadFull, and it simplifies the code
not to do so.  With the shorter names and inverted bool,
I think this can simplify to:

if cr.n == 0 {
    cr.n, cr.open = <-cr.ch
}
if !cr.open {
    return 0, os.EOF
}
if len(p) > cr.n {
    p = p[:cr.n]
}
n, err := cr.r.Read(p)
cr.n -= n
return n, err

That said, you never close the channel so you can
simplify further:

if cr.n == 0 {
    cr.n = <-cr.ch
}
if len(p) > cr.n {
    p = p[:cr.n]
}
n, err := cr.r.Read(p)
cr.n -= n
return n, err

[willchan, 2011-05-31 15:41:06]

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go#n...
src/pkg/http/spdy/framer.go:50: bytesAvailable int
On 2011/05/31 15:01:49, rsc wrote:
> I suggest
> 
> n int
> open bool
> 

Done.

http://codereview.appspot.com/4530089/diff/2001/src/pkg/http/spdy/framer.go#n...
src/pkg/http/spdy/framer.go:55: if cr.bytesAvailable == 0 && !cr.eof {
On 2011/05/31 15:01:49, rsc wrote:
> Various comments.  If you invert the bool as above,
> you can use it directly in the receive.  Read and ReadFull
> are different operations; Read should in general not be
> implemented by calling ReadFull, and it simplifies the code
> not to do so.  With the shorter names and inverted bool,
> I think this can simplify to:
> 
> if cr.n == 0 {
>     cr.n, cr.open = <-cr.ch
> }
> if !cr.open {
>     return 0, os.EOF
> }
> if len(p) > cr.n {
>     p = p[:cr.n]
> }
> n, err := cr.r.Read(p)
> cr.n -= n
> return n, err
> 
> That said, you never close the channel so you can
> simplify further:
> 
> if cr.n == 0 {
>     cr.n = <-cr.ch
> }
> if len(p) > cr.n {
>     p = p[:cr.n]
> }
> n, err := cr.r.Read(p)
> cr.n -= n
> return n, err
> 

Done. Thanks, this is indeed much simpler.

[peterGo, 2011-05-31 16:59:04]

Will,

Follow the Go contribution guidelines and add "Fixes issue 1884." at the end of
the CL description.

The special sentence “Fixes issue 1884.” associates the change with issue 1884
in the Go issue tracker. When this change is eventually submitted, the issue
tracker will automatically mark the issue as fixed.

Make a change, Contribution guidelines.
http://golang.org/doc/contribute.html

Peter

[willchan, 2011-05-31 17:02:21]

Oops, I missed that, thanks! I've changed the first line of the description as
well.

On 2011/05/31 16:59:04, peterGo wrote:
> Will,
> 
> Follow the Go contribution guidelines and add "Fixes issue 1884." at the end
of
> the CL description.
> 
> The special sentence “Fixes issue 1884.” associates the change with issue 1884
> in the Go issue tracker. When this change is eventually submitted, the issue
> tracker will automatically mark the issue as fixed.
> 
> Make a change, Contribution guidelines.
> http://golang.org/doc/contribute.html
> 
> Peter

[peterGo, 2011-05-31 17:03:56]

The first line of the CL description should be something like: "spdy: Fix data
race in header decompression."

The first line of the change description is conventionally a one-line summary of
the change, prefixed by the primary affected package, and is used as the subject
for code review mail; the rest of the description elaborates.

Make a change, Contribution guidelines.
http://golang.org/doc/contribute.html

[willchan, 2011-05-31 17:09:44]

Didn't I already do that? What don't you like about:
"http/spdy: Fix data race in header decompression."?

On Tue, May 31, 2011 at 5:03 PM,  <go.peter.90@gmail.com> wrote:
> The first line of the CL description should be something like: "spdy:
> Fix data race in header decompression."
>
> The first line of the change description is conventionally a one-line
> summary of the change, prefixed by the primary affected package, and is
> used as the subject for code review mail; the rest of the description
> elaborates.
>
> Make a change, Contribution guidelines.
> http://golang.org/doc/contribute.html
>
> http://codereview.appspot.com/4530089/
>

[peterGo, 2011-05-31 17:18:38]

I filed issue 1884. After applying this CL, I now get:

$ hg id
9449a15e4ac4+ tip

$ env | grep '^GO'
GOBIN=/home/peter/spdy/bin
GOARCH=amd64
GOMAXPROCS=4
GOROOT=/home/peter/spdy
GOOS=linux

test http/spdy
TEST FAIL http/spdy
make[1]: Entering directory `/home/peter/spdy/src/pkg/http/spdy'
gotest -test.short -test.timeout=120
rm -f _test/http/spdy.a
6g  -o _gotest_.6 framer.go protocol.go  framer_test.go
rm -f _test/http/spdy.a
gopack grc _test/http/spdy.a _gotest_.6 
--- FAIL: spdy.TestCompressionContextAcrossFrames (0.00 seconds)
	expected SynStreamFrame; got *spdy.DataFrame &{8960 0 []}
--- FAIL: spdy.TestMultipleSPDYFrames (0.00 seconds)
	Parsed incorrect frame type.
FAIL
gotest: "./6.out -test.short=true -test.timeout=120" failed: exit status 1
make[1]: *** [testshort] Error 2
make[1]: Leaving directory `/home/peter/spdy/src/pkg/http/spdy'
make: *** [http/spdy.testshort] Error 1

Peter

[willchan, 2011-05-31 17:20:18]

Thanks for testing this! Hm, I don't seem to be able to repro on my
MBP. I'll try this on my Linux workstation and get back to you.

On Tue, May 31, 2011 at 5:18 PM,  <go.peter.90@gmail.com> wrote:
> I filed issue 1884. After applying this CL, I now get:
>
> $ hg id
> 9449a15e4ac4+ tip
>
> $ env | grep '^GO'
> GOBIN=/home/peter/spdy/bin
> GOARCH=amd64
> GOMAXPROCS=4
> GOROOT=/home/peter/spdy
> GOOS=linux
>
> test http/spdy
> TEST FAIL http/spdy
> make[1]: Entering directory `/home/peter/spdy/src/pkg/http/spdy'
> gotest -test.short -test.timeout=120
> rm -f _test/http/spdy.a
> 6g  -o _gotest_.6 framer.go protocol.go  framer_test.go
> rm -f _test/http/spdy.a
> gopack grc _test/http/spdy.a _gotest_.6
> --- FAIL: spdy.TestCompressionContextAcrossFrames (0.00 seconds)
>        expected SynStreamFrame; got *spdy.DataFrame &{8960 0 []}
> --- FAIL: spdy.TestMultipleSPDYFrames (0.00 seconds)
>        Parsed incorrect frame type.
> FAIL
> gotest: "./6.out -test.short=true -test.timeout=120" failed: exit status
> 1
> make[1]: *** [testshort] Error 2
> make[1]: Leaving directory `/home/peter/spdy/src/pkg/http/spdy'
> make: *** [http/spdy.testshort] Error 1
>
> Peter
>
> http://codereview.appspot.com/4530089/
>

[rsc, 2011-05-31 17:20:42]

please s/Fix/fix/ in the CL description.
code looks fine but waiting to hear resolution
of go.peter.90's failure case.

[peterGo, 2011-05-31 17:23:58]

For GOMAXPROCS=1, ./all.bash succeeds.

$ env | grep '^GO'
GOBIN=/home/peter/spdy/bin
GOARCH=amd64
GOMAXPROCS=1
GOROOT=/home/peter/spdy
GOOS=linux

ALL TESTS PASSED
---
Installed Go for linux/amd64 in /home/peter/spdy.
Installed commands in /home/peter/spdy/bin.
The compiler is 6g.

Peter

[peterGo, 2011-05-31 17:35:00]

Has Albert Strasheim tested this CL?

http/spdy test failure
http://groups.google.com/group/golang-dev/browse_thread/thread/ada97a7289e7cac4

Peter

[rsc, 2011-05-31 17:36:15]

I don't think Albert needs to test it if you have
a failure on your machine already...

[bradfitz, 2011-05-31 18:33:45]

LGTM

Cool. Glad corkedReader ended up working.

I can't get the test to fail on Linux with GOMAXPROCS=1, 2, or 5 and a few
hundreds runs in a loop.

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go#...
src/pkg/http/spdy/framer.go:96: func (f *Framer)
initHeaderDecompression(payloadSize int) os.Error {
this function has a weird name now.

it's called "init" but it takes an action each time it's called now:  sending to
headerReader.ch.

could this be named something more clear?

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go#...
src/pkg/http/spdy/framer.go:101: f.headerReader.ch = make(chan int, 1)
Could also do:

f.headerReader = newCorkedReader()

or:

f.headerReader = corkedReader{r: f.r, ch: make(chan int, 1)}

[willchan, 2011-05-31 19:02:24]

I've changed the changelist description again and addressed Brad's comments. But
this shouldn't have changed the logic of the code, so if it's failing for Peter,
that's a problem. Peter, can you try the latest patchset and see if it still
fails for you?

I haven't tested on Linux yet, but it'l probably have to wait until tomorrow for
me.

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go#...
src/pkg/http/spdy/framer.go:96: func (f *Framer)
initHeaderDecompression(payloadSize int) os.Error {
On 2011/05/31 18:33:45, bradfitz wrote:
> this function has a weird name now.
> 
> it's called "init" but it takes an action each time it's called now:  sending
to
> headerReader.ch.
> 
> could this be named something more clear?

Renamed to uncorkHeaderDecompressor(). Not sure if this makes more sense now.
Let me know what you think.

http://codereview.appspot.com/4530089/diff/11001/src/pkg/http/spdy/framer.go#...
src/pkg/http/spdy/framer.go:101: f.headerReader.ch = make(chan int, 1)
On 2011/05/31 18:33:45, bradfitz wrote:
> Could also do:
> 
> f.headerReader = newCorkedReader()
> 
> or:
> 
> f.headerReader = corkedReader{r: f.r, ch: make(chan int, 1)}
> 

Done.

[bradfitz, 2011-05-31 20:11:21]

LGTM

I've tested this on Linux and can't reproduce any errors.  I'm happy submitting
now, unless you want me to wait.

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go#newc...
src/pkg/http/spdy/framer.go:102: f.headerReader.ch <- payloadSize
minor, and probably won't make much difference, but you could avoid this channel
send and instead just create the corkedReader with n set to payloadSize.  might
avoid some context switching in the common case.

[rsc, 2011-05-31 20:17:31]

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go#newc...
src/pkg/http/spdy/framer.go:102: f.headerReader.ch <- payloadSize
On 2011/05/31 20:11:22, bradfitz wrote:
> minor, and probably won't make much difference, but you could avoid this
channel
> send and instead just create the corkedReader with n set to payloadSize. 
might
> avoid some context switching in the common case.

isn't the common case headerDecompressor != nil ?

[bradfitz, 2011-05-31 20:23:40]

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go#newc...
src/pkg/http/spdy/framer.go:102: f.headerReader.ch <- payloadSize
On 2011/05/31 20:17:31, rsc wrote:
> On 2011/05/31 20:11:22, bradfitz wrote:
> > minor, and probably won't make much difference, but you could avoid this
> channel
> > send and instead just create the corkedReader with n set to payloadSize. 
> might
> > avoid some context switching in the common case.
> 
> isn't the common case headerDecompressor != nil ?

I guess the common case depends on the longevity of the connections.  Not a big
deal either way but seems unnecessary to send + receive on a channel if you
could instead initialize a value to n instead of 0.

[willchan, 2011-05-31 20:26:48]

If you're fine submitting it, then I'm fine with it too.

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go
File src/pkg/http/spdy/framer.go (right):

http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go#newc...
src/pkg/http/spdy/framer.go:102: f.headerReader.ch <- payloadSize
On 2011/05/31 20:23:40, bradfitz wrote:
> On 2011/05/31 20:17:31, rsc wrote:
> > On 2011/05/31 20:11:22, bradfitz wrote:
> > > minor, and probably won't make much difference, but you could avoid this
> > channel
> > > send and instead just create the corkedReader with n set to payloadSize. 
> > might
> > > avoid some context switching in the common case.
> > 
> > isn't the common case headerDecompressor != nil ?
> 
> I guess the common case depends on the longevity of the connections.  Not a
big
> deal either way but seems unnecessary to send + receive on a channel if you
> could instead initialize a value to n instead of 0.

Yeah, I think the common case will be a longer-lived connection (SPDY's after
all designed to multiplex multiple streams on a single connection). That said,
Brad's right that it's unnecessary to send + receive on the channel. I only did
it to keep the code symmetric, but it was unnecessary. I've changed it now.

[bradfitz, 2011-05-31 20:36:39]

Russ,

Do we need to be concerned about flate's inflate goroutine's Read blocking
forever on the channel and thus never being shut down or GC'd?  I know sends
can block goroutines forever but I can't remember reads.

If so, we should probably close(corkedReader.ch) at some point, and have
corkedReader know the always return an error once/after the channel is
closed?


On Tue, May 31, 2011 at 1:26 PM, <willchan@chromium.org> wrote:

> If you're fine submitting it, then I'm fine with it too.
>
>
>
> http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go
> File src/pkg/http/spdy/framer.go (right):
>
>
>
http://codereview.appspot.com/4530089/diff/4/src/pkg/http/spdy/framer.go#newc...
> src/pkg/http/spdy/framer.go:102: f.headerReader.ch <- payloadSize
> On 2011/05/31 20:23:40, bradfitz wrote:
>
>> On 2011/05/31 20:17:31, rsc wrote:
>> > On 2011/05/31 20:11:22, bradfitz wrote:
>> > > minor, and probably won't make much difference, but you could
>>
> avoid this
>
>> > channel
>> > > send and instead just create the corkedReader with n set to
>>
> payloadSize.
>
>> > might
>> > > avoid some context switching in the common case.
>> >
>> > isn't the common case headerDecompressor != nil ?
>>
>
>  I guess the common case depends on the longevity of the connections.
>>
> Not a big
>
>> deal either way but seems unnecessary to send + receive on a channel
>>
> if you
>
>> could instead initialize a value to n instead of 0.
>>
>
> Yeah, I think the common case will be a longer-lived connection (SPDY's
> after all designed to multiplex multiple streams on a single
> connection). That said, Brad's right that it's unnecessary to send +
> receive on the channel. I only did it to keep the code symmetric, but it
> was unnecessary. I've changed it now.
>
>
> http://codereview.appspot.com/4530089/
>

[bradfitz, 2011-05-31 20:44:51]

On Tue, May 31, 2011 at 1:36 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Russ,
>
> Do we need to be concerned about flate's inflate goroutine's Read blocking
> forever on the channel and thus never being shut down or GC'd?  I know sends
> can block goroutines forever but I can't remember reads.
>
> If so, we should probably close(corkedReader.ch) at some point, and have
> corkedReader know the always return an error once/after the channel is
> closed?
>

It seems a Read will keep a goroutine alive:

package main

import (
        "runtime"
        "time"
)

func foo() {
        ch := make(chan int)
        go func() {
                <-ch
        }()
}

func main() {
        go foo()
        time.Sleep(1e9)
        runtime.GC()
        runtime.GC()
        time.Sleep(10e9)   // SIGQUIT it here
}

And see:

goroutine 3 [4]:
runtime.gosched+0x5c
/usr/local/google/home/bradfitz/go/src/pkg/runtime/proc.c:603
runtime.gosched()
runtime.chanrecv+0x458
/usr/local/google/home/bradfitz/go/src/pkg/runtime/chan.c:329
runtime.chanrecv(0xf84000cb90, 0x7f9f02b9cf98, 0x0, 0x0, 0x0, ...)
runtime.chanrecv1+0x4a
/usr/local/google/home/bradfitz/go/src/pkg/runtime/chan.c:424
runtime.chanrecv1(0xf84000cb90, 0x0)
main._func_001+0x28 /usr/local/google/home/bradfitz/sendblock.go:11
main._func_001(0xf8400000f8, 0x40af8d)
runtime.goexit /usr/local/google/home/bradfitz/go/src/pkg/runtime/proc.c:178
runtime.goexit()
----- goroutine created by -----
main.foo+0x7b /usr/local/google/home/bradfitz/sendblock.go:12

[rsc, 2011-05-31 20:45:22]

> Do we need to be concerned about flate's inflate goroutine's Read blocking
> forever on the channel and thus never being shut down or GC'd?

Right now it probably will but I think that is a bug in flate.

[bradfitz, 2011-05-31 20:47:13]

On Tue, May 31, 2011 at 1:45 PM, Russ Cox <rsc@golang.org> wrote:

> > Do we need to be concerned about flate's inflate goroutine's Read
> blocking
> > forever on the channel and thus never being shut down or GC'd?
>
> Right now it probably will but I think that is a bug in flate.
>

How so?  flate.NewReader effectively takes ownership of a reader you give it
and reads until the end.  In this case we're playing a trick on it and
"giving" it a reader that's really a coordinated reader that we're sharing.

And in this case, flate's reader never sees EOF?

[rsc, 2011-05-31 20:48:12]

On Tue, May 31, 2011 at 16:47, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Tue, May 31, 2011 at 1:45 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> > Do we need to be concerned about flate's inflate goroutine's Read
>> > blocking
>> > forever on the channel and thus never being shut down or GC'd?
>>
>> Right now it probably will but I think that is a bug in flate.
>
> How so?  flate.NewReader effectively takes ownership of a reader you give it
> and reads until the end.  In this case we're playing a trick on it and
> "giving" it a reader that's really a coordinated reader that we're sharing.
> And in this case, flate's reader never sees EOF?

I think it was probably a mistake to make flate's reader
do background reads.  We've had multiple bugs because
of this.  bufio doesn't do background reads.

Russ

[bradfitz, 2011-05-31 20:56:18]

On Tue, May 31, 2011 at 1:48 PM, Russ Cox <rsc@golang.org> wrote:

> On Tue, May 31, 2011 at 16:47, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > On Tue, May 31, 2011 at 1:45 PM, Russ Cox <rsc@golang.org> wrote:
> >>
> >> > Do we need to be concerned about flate's inflate goroutine's Read
> >> > blocking
> >> > forever on the channel and thus never being shut down or GC'd?
> >>
> >> Right now it probably will but I think that is a bug in flate.
> >
> > How so?  flate.NewReader effectively takes ownership of a reader you give
> it
> > and reads until the end.  In this case we're playing a trick on it and
> > "giving" it a reader that's really a coordinated reader that we're
> sharing.
> > And in this case, flate's reader never sees EOF?
>
> I think it was probably a mistake to make flate's reader
> do background reads.  We've had multiple bugs because
> of this.  bufio doesn't do background reads.
>
>
So what should we do for now?  spdy as-is, under moderate usage, will leak
memory it seems.

Can we do a channel close for now in a runtime.SetFinalizer with a "TODO:
remove this once flate doesn't read forever in the background" ?

[rsc, 2011-05-31 21:03:49]

On Tue, May 31, 2011 at 16:56, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> So what should we do for now?

check it in as is and don't worry about the memory.
i will fix flat.e

[bradfitz, 2011-05-31 21:06:14]

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

http/spdy: fix data race in header decompression.

flate's reader greedily reads from the shared io.Reader in Framer. This leads to
a data race on Framer.r. Fix this by providing a corkedReader to
zlib.NewReaderDict(). We uncork the reader and allow it to read the number of
bytes in the compressed payload.

Fixes issue 1884.

R=bradfitz, rsc, go.peter.90
CC=golang-dev
http://codereview.appspot.com/4530089

Committer: Brad Fitzpatrick <bradfitz@golang.org>