code review 4969056: io: add a Tee

io: add a Tee

io.MultiReader is like unix cat, and MultiWriter is like unix tee, but it is a writer.  I needed a reader that did tee (for forwarding an html PUT request to a bunch of other servers).

[lvd, 2011-09-03 11:18:58]

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

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

[hector, 2011-09-03 15:32:05]

Snap!  I added something like this a little over a week ago.
Mine's called io.TeeReader, but yours looks like it's more advanced.
Qualitatively, the main difference between yours and mine is that my
Tee writes, whereas your's buffers the data and allows multiple
clients to read from it.
Currently it looks possible to replicate the functionality you're
proposing with the following chain:
Reader --> TeeReader   --> Tap 0
               |
           MultiWriter --> Pipe n --> Tap n

On Sep 3, 12:18 pm, lvd@google.com wrote:
> Reviewers: adg,
>
> Message:
> Hello adg@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change tohttps://go.googlecode.com/hg/
>
> Description:
>         io: add a Tee
>
> io.MultiReader is like unix cat, and MultiWriter is like unix tee, but
> it is a writer.  I needed a reader that did tee (for forwarding an html
> PUT request to a bunch of other servers).
>
> Please review this athttp://codereview.appspot.com/4969056/
>
> Affected files:
>    M src/pkg/io/Makefile
>    A src/pkg/io/tee.go
>    A src/pkg/io/tee_test.go

[lvd, 2011-09-04 08:54:40]

i dont want the slowest reader to block the others
On Sep 3, 2011 5:32 PM, "Hector Chu" <hectorchu@gmail.com> wrote:
> Snap! I added something like this a little over a week ago.
> Mine's called io.TeeReader, but yours looks like it's more advanced.
> Qualitatively, the main difference between yours and mine is that my
> Tee writes, whereas your's buffers the data and allows multiple
> clients to read from it.
> Currently it looks possible to replicate the functionality you're
> proposing with the following chain:
> Reader --> TeeReader --> Tap 0
> |
> MultiWriter --> Pipe n --> Tap n
>
> On Sep 3, 12:18 pm, lvd@google.com wrote:
>> Reviewers: adg,
>>
>> Message:
>> Hello adg@golang.org (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change tohttps://go.googlecode.com/hg/
>>
>> Description:
>>         io: add a Tee
>>
>> io.MultiReader is like unix cat, and MultiWriter is like unix tee, but
>> it is a writer.  I needed a reader that did tee (for forwarding an html
>> PUT request to a bunch of other servers).
>>
>> Please review this athttp://codereview.appspot.com/4969056/
>>
>> Affected files:
>>    M src/pkg/io/Makefile
>>    A src/pkg/io/tee.go
>>    A src/pkg/io/tee_test.go

[hector, 2011-09-04 09:20:46]

Ok.  But just for the record this criticism can be addressed by making
the pipes buffered (though there is still the problem that tap 0
drives the I/O).
Can we think up a new name for this reader, to avoid confusion with
the TeeReader already in io?  What about DupReader?
Or else rename the existing TeeReader to WriteReader.

On 4 September 2011 09:54, Luuk van Dijk <lvd@google.com> wrote:
> i dont want the slowest reader to block the others
>
> On Sep 3, 2011 5:32 PM, "Hector Chu" <hectorchu@gmail.com> wrote:
>> Snap! I added something like this a little over a week ago.
>> Mine's called io.TeeReader, but yours looks like it's more advanced.
>> Qualitatively, the main difference between yours and mine is that my
>> Tee writes, whereas your's buffers the data and allows multiple
>> clients to read from it.
>> Currently it looks possible to replicate the functionality you're
>> proposing with the following chain:
>> Reader --> TeeReader --> Tap 0
>> |
>> MultiWriter --> Pipe n --> Tap n
>>
>> On Sep 3, 12:18 pm, lvd@google.com wrote:
>>> Reviewers: adg,
>>>
>>> Message:
>>> Hello adg@golang.org (cc: golang-dev@googlegroups.com),
>>>
>>> I'd like you to review this change tohttps://go.googlecode.com/hg/
>>>
>>> Description:
>>>         io: add a Tee
>>>
>>> io.MultiReader is like unix cat, and MultiWriter is like unix tee, but
>>> it is a writer.  I needed a reader that did tee (for forwarding an html
>>> PUT request to a bunch of other servers).
>>>
>>> Please review this athttp://codereview.appspot.com/4969056/
>>>
>>> Affected files:
>>>    M src/pkg/io/Makefile
>>>    A src/pkg/io/tee.go
>>>    A src/pkg/io/tee_test.go
>

[lvd, 2011-09-04 13:34:08]

On Sun, Sep 4, 2011 at 11:20, Hector Chu <hectorchu@gmail.com> wrote:

> Ok.  But just for the record this criticism can be addressed by making
> the pipes buffered (though there is still the problem that tap 0
> drives the I/O).
> Can we think up a new name for this reader, to avoid confusion with
> the TeeReader already in io?  What about DupReader?
> Or else rename the existing TeeReader to WriteReader.
>

if i implement the buffer limiting, (see the todo, won't be hard) it can do
everything yours can do (if you'd set the buffer limit to zero, it would not
do new reads from in until the buffer is empty, meaning read by all taps),
so why don't i do that and it can replace TeeReader altogether?

otherwise this can go in ioutil for all i care.

/L

[rsc, 2011-09-04 16:33:21]

I think the existing io.TeeReader should suffice.

The Tee in this CL is very customized: it's trying
to do about three or four different things, all
of which may or may not apply to a particular
job.  The existing TeeReader handles just the
fundamental operation of splitting off Reads
into a secondary output stream.  The rest -
writing to multiple streams, buffering, taps,
and so on, can and should be separate pieces.
io.MultiWriter is one such piece.  If there are
other fundamental building blocks to spilt off
and add as separate data types, great.

Russ

[lvd, 2011-09-04 18:50:22]

On Sun, Sep 4, 2011 at 18:33, Russ Cox <rsc@golang.org> wrote:

> I think the existing io.TeeReader should suffice.
>
>
Ah the old "we already have that/we don't need it/it's not gonna work"
triple mantra.  I know how to handle those, i work at Google....(*)


> The Tee in this CL is very customized: it's trying
> to do about three or four different things, all
> of which may or may not apply to a particular
> job.


three or four eh? Bollocks, with all due respect. it is not customized at
all and  it does exactly one thing: allow you to read multiple copies of a
stream.  It also does it well, avoiding spurious copies and avoiding
introducing new semantics on the Reader class.  The code inside is maybe not
Turing Award material, but it's easy to get wrong.

The existing TeeReader handles just the
> fundamental operation of splitting off Reads
> into a secondary output stream.


The existing io.TeeReader on the other hand adds a few lines that anyone
that needs them could add in their own code, and introduces the guarantee
that you can't read something unless you can also write it somewhere.


>  The rest -
> writing to multiple streams, buffering, taps,
> and so on, can and should be separate pieces.
>

if you want to use the existing TeeReader to copy a Reader to 5 other
Readers you'll find yourself doing spurious copies and having to fit Pipes
in.  If you buffer the common input you end up writing much the same class
as i just did.   You should be able to use Readers without having to
introduce writers.


> io.MultiWriter is one such piece.  If there are
> other fundamental building blocks to spilt off
> and add as separate data types, great.


The MultiWriter is useless for this purpose,  and as noted the name pair
MultiWriter and MultiReader suggest a symmetry that's not there.



(*)...  but i also know how it always ends, so sod it, i'll scrap this.

/L

[lvd, 2011-09-04 18:54:15]

*** Abandoned ***

[adg, 2011-09-04 23:39:41]

On 5 September 2011 02:33, Russ Cox <rsc@golang.org> wrote:
> I think the existing io.TeeReader should suffice.
>
> The Tee in this CL is very customized: it's trying
> to do about three or four different things, all
> of which may or may not apply to a particular
> job.  The existing TeeReader handles just the
> fundamental operation of splitting off Reads
> into a secondary output stream.  The rest -
> writing to multiple streams, buffering, taps,
> and so on, can and should be separate pieces.
> io.MultiWriter is one such piece.  If there are
> other fundamental building blocks to spilt off
> and add as separate data types, great.

By "customized" are you referring to its buffering? What if you could
specify the buffer size? (0, n, or -1 for unbounded)

This has a nice interface, is efficient, and is the right tool for
many tasks. You could replicate it with the combination of several
pieces, but it would be clumsy and less efficient. At 100 lines this
doesn't seem like a code burden.

[rsc, 2011-09-05 12:01:04]

I took another look at the code.
You are right: it does less than I thought.

That said, it is not like tee(1).
Tee(1) is about writing out a copy as the data
flows through, which is what TeeReader does,
and does well.  Also, I disagree strongly that
TeeReader introduces "new semantics on the
Reader class."

What is the context in which this CL's Tee is useful?
You have written off the fact that it requires an
infinite buffer as a TODO, but that is very
fundamental to the way you've laid it out,
since you also said you don't want the slowest
reader to block the others.  You can't both have
a buffer limit and keep the slowest reader from
blocking the others.

If there is no buffer limit, then I would argue that

data, err := ioutil.ReadAll(r)
tap1 := bytes.NewBuffer(data)
tap2 := bytes.NewBuffer(data)
tap3 := bytes.NewBuffer(data)

works as nicely with less code.

If there is a limit, then you have to do something
to keep them all in sync.  One very good way to
do this is to turn the I/O around so that the data is
being pushed out to the sinks, by using a Writer
and maybe even an io.Pipe.

Most fundamentally, I don't think this is a very
common situaton to be in.  Programs typically
use Readers for input and Writers for output.
That means that data moving toward the program
is a Reader and data moving away is a Writer.
Pipe means everything can be converted the
other way for the general case and lets the APIs
focus on the common cases.  The common case
is tee(1), which is TeeReader.

Russ