code review 2137041: netchan: use acknowledgements on export send.

netchan: use acknowledgements on export send.

[r, 2010-09-02 00:48:03]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[r, 2010-09-02 00:49:42]

That was not meant to be mailed.  I did hg mail when I meant hg change.

Please ignore for now.

-rob

[rsc1, 2010-09-02 01:17:53]

(removing reviewers)

[r, 2010-09-02 05:36:30]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[r, 2010-09-02 05:42:37]

I did hg change and added rsc, but that act did not add rsc to the reviewer
field.  A minor bug in codereview, I suspect.

Adding him by hand now.

[rsc1, 2010-09-02 15:49:09]

http://codereview.appspot.com/2137041/diff/5001/6001
File src/pkg/netchan/common.go (right):

http://codereview.appspot.com/2137041/diff/5001/6001#newcode59
src/pkg/netchan/common.go:59: // a handy marshaling place for its data.
d

http://codereview.appspot.com/2137041/diff/5001/6001#newcode108
src/pkg/netchan/common.go:108: } else {
should be
}
if err != nil {

(err changed)

http://codereview.appspot.com/2137041/diff/5001/6001#newcode115
src/pkg/netchan/common.go:115: // Drain waits until all messages sent from this
exporter/importer, possibly
Is this intended to be a public function?
This nice doc comment won't show up in godoc,
because it doesn't handle embedded types.

http://codereview.appspot.com/2137041/diff/5001/6001#newcode120
src/pkg/netchan/common.go:120: startTime := time.Nanoseconds()
I think the semantics here should be that Drain waits
until all messages sent before the call to Drain have
been received.  Otherwise Drain is not useful except
when tearing down a connection.  If each message
is numbered, it should be a matter of checking what
the last sent number is on entry to the function,
and waiting for the "all acked up to here" number
to catch up.  I don't know how to deal with the 
buffered channel length part though.  Maybe it can
be defined not to worry about that.  The point is really
to provide something like the sync guarantees of
ordinary channels, which would also ignore the buffer.

Maybe s/Drain/Sync/ too.

http://codereview.appspot.com/2137041/diff/5001/6002
File src/pkg/netchan/export.go (right):

http://codereview.appspot.com/2137041/diff/5001/6002#newcode86
src/pkg/netchan/export.go:86: // run Manages sends and receives for a single
client.  For each (client Recv) request,
s/M/m/

http://codereview.appspot.com/2137041/diff/5001/6002#newcode98
src/pkg/netchan/export.go:98: // TODO: tear down connection
d

http://codereview.appspot.com/2137041/diff/5001/6002#newcode105
src/pkg/netchan/export.go:105: // TODO: tear down connection
d

http://codereview.appspot.com/2137041/diff/5001/6002#newcode128
src/pkg/netchan/export.go:128: // to be setn in order.  Therefore receipt of
acknowledgement N means
s/setn/sent/

[r, 2010-09-02 21:13:50]

> http://codereview.appspot.com/2137041/diff/5001/6001#newcode120
I don't know how to deal with the
> buffered channel length part though.  Maybe it can
> be defined not to worry about that.  The point is really
> to provide something like the sync guarantees of
> ordinary channels, which would also ignore the buffer.

That's a tough one.  I put this in and was immediately happy with it.
It means something that wants to deliver an item to the first caller
can fire and forget (call Drain).  Without the length check, messages
that have been sent but not seen by any client are just sitting in the
channel buffers.    In particular - this is the critical part - using
the sequence numbers alone to decide when we're done means we miss all
the messages
that the server has sent but the client has not yet requested.  Since
from the server's point of view, the distinction is invisible, it
seems wrong to make Drain distinguish the two states.

> Maybe s/Drain/Sync/ too.

Or flush. I went with drain because as written it's not a sync and
it's not a flush as usually understood (see above).

-rob

[r, 2010-09-02 21:41:46]

Maybe we want both, a Drain that means 'empty everything' and a Sync
that means 'wait until the current clients are caught up to this
point'.

-rob

[r, 2010-09-03 01:43:53]

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

Please take another look.

[r, 2010-09-04 12:32:21]

ping

[rsc1, 2010-09-04 12:48:41]

LGTM

[r, 2010-09-04 13:42:01]

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

netchan: use acknowledgements on export send.
Also add exporter.Drain() to wait for completion.
This makes it possible for an Exporter to fire off a message
and wait (by calling Drain) for the message to be received,
even if a client has yet to call to retrieve it.

Once this design is settled, I'll do the same for import send.

Testing strategies welcome.  I have some working stand-alone
tests.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2137041

Committer: Rob Pike <r@golang.org>