code review 1764043: io: MultiReader and MultiWriter

io: MultiReader and MultiWriter

Little helpers I've found useful.

[brad_danga_com, 2010-07-27 23:00:54]

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

I'd like you to review this change.

[brad_danga_com, 2010-07-27 23:02:38]

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

Please take another look.

[r, 2010-07-28 00:03:48]

these are really nice. my comments are all minor

http://codereview.appspot.com/1764043/diff/6001/7002
File src/pkg/io/multi_reader.go (right):

http://codereview.appspot.com/1764043/diff/6001/7002#newcode39
src/pkg/io/multi_reader.go:39: // the provided input readers.  They're read
sequentially.  os.EOF is
English sentences start with upper-case letters.  Also I think it can return
lots of os.EOFs at the end.

How about

Once all inputs are drained, Read will return os.EOF.

http://codereview.appspot.com/1764043/diff/6001/7003
File src/pkg/io/multi_reader_test.go (right):

http://codereview.appspot.com/1764043/diff/6001/7003#newcode25
src/pkg/io/multi_reader_test.go:25: t.Errorf("Didn't get 'fo', but: %s", buf)
always nice to use %q in tests when reported unexpected data. also the message
has inconsistent style.

expected 'fo'; got %q

http://codereview.appspot.com/1764043/diff/6001/7003#newcode34
src/pkg/io/multi_reader_test.go:34: t.Errorf("read %d bytes, not 5", n)
ditto

http://codereview.appspot.com/1764043/diff/6001/7003#newcode38
src/pkg/io/multi_reader_test.go:38: }
ditto

(you're even missing a verb here)

http://codereview.appspot.com/1764043/diff/6001/7004
File src/pkg/io/multi_writer.go (right):

http://codereview.appspot.com/1764043/diff/6001/7004#newcode32
src/pkg/io/multi_writer.go:32: // MultiWriter creates a writer which duplicates
its writes to all the
s/which/that/

http://codereview.appspot.com/1764043/diff/6001/7005
File src/pkg/io/multi_writer_test.go (right):

http://codereview.appspot.com/1764043/diff/6001/7005#newcode35
src/pkg/io/multi_writer_test.go:35: t.Error("Bogus sha1 value")
s/B/b/ or better: incorrect sha1 value.

http://codereview.appspot.com/1764043/diff/6001/7005#newcode39
src/pkg/io/multi_writer_test.go:39: t.Error("Unexpected sink output: %v",
sink.String())
s/U/u/ or better
expected %q; got %q

[adg, 2010-07-28 00:31:40]

LGTM

except:

http://codereview.appspot.com/1764043/diff/6001/7004
File src/pkg/io/multi_writer.go (right):

http://codereview.appspot.com/1764043/diff/6001/7004#newcode18
src/pkg/io/multi_writer.go:18: if err != nil {
if ew != nil ?

(or am I missing something?)

[brad_danga_com, 2010-07-28 03:46:38]

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

Please take another look.

[r2, 2010-07-28 04:07:15]

LGTM

anyone else?

[rsc1, 2010-07-28 04:23:27]

http://codereview.appspot.com/1764043/diff/15001/16002
File src/pkg/io/multi_reader.go (right):

http://codereview.appspot.com/1764043/diff/15001/16002#newcode15
src/pkg/io/multi_reader.go:15: func (cr *multiReader) Read(p []byte) (n int, err
os.Error) {
An io.Reader is not required to attempt to fill the buffer being
passed in, but it looks like this routine does just that.
If one of the concatenated readers is, say, a network,
this might be undesirable.  I think this code should try
each reader in order, retiring it if it returns EOF.
The first Read that doesn't return EOF would be the return value.
(For forcing a full buffer read there's io.ReadFull.)

http://codereview.appspot.com/1764043/diff/15001/16003
File src/pkg/io/multi_reader_test.go (right):

http://codereview.appspot.com/1764043/diff/15001/16003#newcode33
src/pkg/io/multi_reader_test.go:33: if n != 5 {
Right, so this should be 2 bytes, and then 3 more.

http://codereview.appspot.com/1764043/diff/15001/16004
File src/pkg/io/multi_writer.go (right):

http://codereview.appspot.com/1764043/diff/15001/16004#newcode17
src/pkg/io/multi_writer.go:17: nw, ew := w.Write(p)
could avoid the temporaries with n, err = w.Write(p)

http://codereview.appspot.com/1764043/diff/15001/16005
File src/pkg/io/multi_writer_test.go (right):

http://codereview.appspot.com/1764043/diff/15001/16005#newcode19
src/pkg/io/multi_writer_test.go:19: mw := MultiWriter(sha1, sink)
If I ever have to debug this code I reserve the right to
replace the sha1 with a second bytes.Buffer.
Inverting sha1 is not one of my strengths.  ☺

[gri, 2010-07-28 04:28:08]

FYI

http://codereview.appspot.com/1764043/diff/15001/16002
File src/pkg/io/multi_reader.go (right):

http://codereview.appspot.com/1764043/diff/15001/16002#newcode7
src/pkg/io/multi_reader.go:7: import (
perhaps remove the ()'s

http://codereview.appspot.com/1764043/diff/15001/16004
File src/pkg/io/multi_writer.go (right):

http://codereview.appspot.com/1764043/diff/15001/16004#newcode8
src/pkg/io/multi_writer.go:8: "os"
perhaps no ()'s

[adg, 2010-07-28 04:28:29]

LGTM

On 28 July 2010 13:46,  <bradfitz@gmail.com> wrote:
> Hello adg, rsc, r (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1764043/show
>

[brad_danga_com, 2010-07-28 08:00:13]

Hello adg, rsc, r, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[dangabrad, 2010-07-28 08:02:54]

I think I've addressed all the comments, notably Russ's about not looping
trying to fill the slice given to Read.

On Wed, Jul 28, 2010 at 1:00 AM, <bradfitz@gmail.com> wrote:

> Hello adg, rsc, r, gri (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1764043/show
>

[rsc1, 2010-07-28 08:35:50]

i'm sorry for bikeshedding this but i think you
can simplify the control flow of Read considerably.

for len(r) > 0 {
    try r[0].Read
    check n>0 && EOF
    return if err != EOF
    r = r[1:]
}
return 0, EOF

what you have now is using the old control
flow to implement the new semantics and it
isn't a good match.

http://codereview.appspot.com/1764043/diff/23001/24004
File src/pkg/io/multi_writer.go (right):

http://codereview.appspot.com/1764043/diff/23001/24004#newcode15
src/pkg/io/multi_writer.go:15: nw, ew := w.Write(p)
why is this nw, ew?
seems like n,err = w.Write(p)
would save a lot of assignments below

[brad_danga_com, 2010-07-28 15:15:19]

Hello adg, rsc, r, gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[bradfitzgoog, 2010-07-28 15:21:19]

Agreed, much better with your suggestions.  Sorry I missed the multi_writer
local variable one earlier.  PTAL.

On Wed, Jul 28, 2010 at 1:35 AM, <rsc@google.com> wrote:

> i'm sorry for bikeshedding this but i think you
> can simplify the control flow of Read considerably.


> for len(r) > 0 {
>    try r[0].Read
>    check n>0 && EOF
>    return if err != EOF
>    r = r[1:]
> }
> return 0, EOF
>
> what you have now is using the old control
> flow to implement the new semantics and it
> isn't a good match.
>
>
>
>
> http://codereview.appspot.com/1764043/diff/23001/24004
>
> File src/pkg/io/multi_writer.go (right):
>
> http://codereview.appspot.com/1764043/diff/23001/24004#newcode15
> src/pkg/io/multi_writer.go:15: nw, ew := w.Write(p)
> why is this nw, ew?
> seems like n,err = w.Write(p)
> would save a lot of assignments below
>
>
> http://codereview.appspot.com/1764043/show
>

[dangabrad, 2010-07-28 15:35:34]

Agreed, much better with your suggestions.  Sorry I missed the multi_writer
local variable one earlier.  PTAL.

On Wed, Jul 28, 2010 at 1:35 AM, <rsc@google.com> wrote:

> i'm sorry for bikeshedding this but i think you
> can simplify the control flow of Read considerably.
>
> for len(r) > 0 {
>    try r[0].Read
>    check n>0 && EOF
>    return if err != EOF
>    r = r[1:]
> }
> return 0, EOF
>
> what you have now is using the old control
> flow to implement the new semantics and it
> isn't a good match.
>
>
>
>
> http://codereview.appspot.com/1764043/diff/23001/24004
>
> File src/pkg/io/multi_writer.go (right):
>
> http://codereview.appspot.com/1764043/diff/23001/24004#newcode15
> src/pkg/io/multi_writer.go:15: nw, ew := w.Write(p)
> why is this nw, ew?
> seems like n,err = w.Write(p)
> would save a lot of assignments below
>
>
> http://codereview.appspot.com/1764043/show
>

[rsc1, 2010-07-28 18:28:18]

LGTM

[rsc, 2010-07-28 18:28:51]

thanks again; these are nice little gems.

[rsc, 2010-07-28 18:30:06]

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

io: MultiReader and MultiWriter

Little helpers I've found useful.

R=adg, rsc, r, gri
CC=golang-dev
http://codereview.appspot.com/1764043

Committer: Russ Cox <rsc@golang.org>

[rog, 2010-08-02 17:01:48]

[sorry for the late response to this - i've been largely offline
for a while]

i like these, but i've got one small reservation: the behaviour of
MultiWriter in the presence of errors.

currently it aborts on the first error encountered.  i suggest that it
should attempt to do all the writes regardless, possibly collecting up
all the error messages encountered along the way.  this means an early
fail does not prevent any data being written.

it also means that a knowledgeable piece of code can determine which
stream the error has occurred on, and react accordingly (for instance
by removing the error'd writer from the slice)

attached is some code to do this.  if it seems reasonable i'll submit
it as a CL.

when making the change, it seemed to me that it was unnecessary to use a
private struct for the MultiWriter - using a slice directly (along the
lines of Vector) avoids the need for a separate constructor function
and makes the parallel with MultiError more obvious.

a similar (and more trivial) change to MultiReader might be
appropriate too.

one other minor nit: wouldn't multireader.go be better than multi_reader.go


On 28 July 2010 19:30,  <rsc@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=339a08a7dc69 ***
>
> io: MultiReader and MultiWriter
>
> Little helpers I've found useful.
>
> R=adg, rsc, r, gri
> CC=golang-dev
> http://codereview.appspot.com/1764043
>
> Committer: Russ Cox <rsc@golang.org>
>
>
> http://codereview.appspot.com/1764043/show
>

[r2, 2010-08-02 20:08:27]

On 03/08/2010, at 3:01 AM, roger peppe wrote:

> one other minor nit: wouldn't multireader.go be better than multi_reader.go

Yes.  Let's start there.  Would you like to put together a CL to rename the
files?  That should be separate from whatever library changes happen.

-rob

[rsc, 2010-08-02 21:12:57]

various disconnected observations

If you're going to shuffle things I'd just
put them both into a single multi.go.
They're similar enough and short enough.

If MultiWriter does return an error, what
does it choose as the number of bytes
written?  You've used zero; maybe that's fine.
It's not really accurate but neither is any
other number.

I'm not convinced it matters enough to say
writing all is better than writing until an error
or vice versa.  I just don't know.

I wonder if the error list concept is worth
putting in os as an implementation of os.Error.

type ErrorList []os.Error

// so *ErrorList implements os.Error
func (e *ErrorList) String() string

// add to the list; if err is an *ErrorList, flatten
func (e *ErrorList) Push(err os.Error)

// maybe?
func (e *ErrorList) Error() os.Error {
    if len(*e) == 0 {
        return nil
    }
    if len(*e) == 1 {
        return (*e)[0]
    }
    return e
}

I've written code that could have used this
as the underlying mechanism.

go/scanner has an ErrorList too but that one
is tailored to parsing and knows how to sort
by line number and probably not able to use
the more general one.

Russ

[r2, 2010-08-02 21:41:26]

that all sounds good.

maybe i'll just do the merge into one file as one CL, then start on  
the list idea.

-rob

[rog, 2010-08-03 08:33:52]

On 2 August 2010 22:12, Russ Cox <rsc@golang.org> wrote:
> If MultiWriter does return an error, what
> does it choose as the number of bytes
> written?  You've used zero; maybe that's fine.
> It's not really accurate but neither is any
> other number.

yes. it's not perfect, but how common is it
to get an error when writing a non-zero number
of bytes? i think zero is the only reasonable
response here. currently the situation is just
as bad: you get a number but there's no
way of telling which writer gave you that number.

> I'm not convinced it matters enough to say
> writing all is better than writing until an error
> or vice versa.  I just don't know.

consider writing log messages both to a file
and to a network connection. when the network
connection goes down, we don't want to lose
a log message written to the file. if the filesystem
fills up, we don't want to lose a log message written
to the net connection. the only way to avoid this
is to make sure that all writes are attempted each
time.

> I wonder if the error list concept is worth
> putting in os as an implementation of os.Error.

i think this is a good idea, but i'm not sure
it's exactly what is needed here. an important
(as i see it) aspect to the error returned
from MultiWriter.Write is that you can
tell which stream yielded the error.
this makes it possible to, for instance,
attach a correct file name to the error message.
it looks like ErrorList assumes that it's
densely filled with errors, and that having
gaps might break things. but maybe it's
ok anyway, as long as ErrorList.Error scans
for a non-nil error rather than using len().

[rsc, 2010-08-03 20:16:57]

>> I wonder if the error list concept is worth
>> putting in os as an implementation of os.Error.
>
> i think this is a good idea, but i'm not sure
> it's exactly what is needed here. an important
> (as i see it) aspect to the error returned
> from MultiWriter.Write is that you can
> tell which stream yielded the error.
> this makes it possible to, for instance,
> attach a correct file name to the error message.
> it looks like ErrorList assumes that it's
> densely filled with errors, and that having
> gaps might break things. but maybe it's
> ok anyway, as long as ErrorList.Error scans
> for a non-nil error rather than using len().

i missed that about the multierror.
seems a bit overengineered really
but i suppose it's fine.  that wouldn't
go into package os.

russ