code review 4548079: compress/flate: do not use background goroutines

compress/flate: do not use background goroutines

Programs expect that Read and Write are synchronous.
The background goroutines make the implementation
a little easier, but they introduce asynchrony that
trips up calling code.  Remove them.

[rsc, 2011-06-01 19:42:25]

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

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

[krasin, 2011-06-01 20:41:38]

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
File src/pkg/compress/flate/deflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate.go:206: // init
Looks like an obsolete comment

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate.go:397: d.fill = (*compressor).fillStore
Not a real comment: I don't understand the construction (*type).method

Could you, please, point me to the reference?

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
File src/pkg/compress/flate/inflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
src/pkg/compress/flate/inflate.go:291: if f.err == os.EOF {
Shouldn't we close f.r here?

[rsc, 2011-06-01 21:15:33]

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
File src/pkg/compress/flate/deflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate.go:206: // init
On 2011/06/01 20:41:38, krasin wrote:
> Looks like an obsolete comment

Done.

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate.go:397: d.fill = (*compressor).fillStore
On 2011/06/01 20:41:38, krasin wrote:
> Not a real comment: I don't understand the construction (*type).method
> 
> Could you, please, point me to the reference?

Sure.  http://golang.org/doc/go_spec.html#Method_expressions
It means the func value associated with the method if you
change the receiver to be a new first argument.

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
File src/pkg/compress/flate/inflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
src/pkg/compress/flate/inflate.go:291: if f.err == os.EOF {
On 2011/06/01 20:41:38, krasin wrote:
> Shouldn't we close f.r here?

No, what got passed to NewReader was an io.Reader,
not an io.ReadCloser.  We *can't* close f.r even if we
wanted to.

Close used to tear down the goroutine associated with
the reader, but now there isn't one.  Probably Close
will go away entirely, but that's a separate CL.

[krasin, 2011-06-01 21:30:49]

LGTM

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
File src/pkg/compress/flate/deflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate.go:397: d.fill = (*compressor).fillStore
On 2011/06/01 21:15:34, rsc wrote:
> On 2011/06/01 20:41:38, krasin wrote:
> > Not a real comment: I don't understand the construction (*type).method
> > 
> > Could you, please, point me to the reference?
> 
> Sure.  http://golang.org/doc/go_spec.html#Method_expressions
> It means the func value associated with the method if you
> change the receiver to be a new first argument.
Thanks.

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
File src/pkg/compress/flate/inflate.go (right):

http://codereview.appspot.com/4548079/diff/6001/src/pkg/compress/flate/inflat...
src/pkg/compress/flate/inflate.go:291: if f.err == os.EOF {
On 2011/06/01 21:15:34, rsc wrote:
> On 2011/06/01 20:41:38, krasin wrote:
> > Shouldn't we close f.r here?
> 
> No, what got passed to NewReader was an io.Reader,
> not an io.ReadCloser.  We *can't* close f.r even if we
> wanted to.
> 
> Close used to tear down the goroutine associated with
> the reader, but now there isn't one.  Probably Close
> will go away entirely, but that's a separate CL.

Thanks, I've missed that it was io.Reader.

[rsc, 2011-06-02 13:32:42]

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

compress/flate: do not use background goroutines

Programs expect that Read and Write are synchronous.
The background goroutines make the implementation
a little easier, but they introduce asynchrony that
trips up calling code.  Remove them.

R=golang-dev, krasin
CC=golang-dev
http://codereview.appspot.com/4548079