code review 2469043: netchan: add new method Hangup to terminate transmissio...

netchan: add new method Hangup to terminate transmission on a channel
Fixes issue 1151.

[r, 2010-10-14 21:44:38]

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

I'd like you to review this change.

[rsc1, 2010-10-15 02:04:28]

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/export.go
File src/pkg/netchan/export.go (right):

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/export.go#newcod...
src/pkg/netchan/export.go:351: // direction is Send, the channel is also closed.
Messages in flight for the channel
I think it would be simpler and just as good to cut the closing.
It doesn't affect remote receivers, since they're on a different machine;
if the local machine has no receivers, it's a no-op;
and if the local machine has receivers, it's not clear you want to stop them.

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/import.go
File src/pkg/netchan/import.go (right):

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/import.go#newcod...
src/pkg/netchan/import.go:208: // direction is Send, the channel is also closed.
Messages in flight for the channel
Same comment.

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/netchan_test.go
File src/pkg/netchan/netchan_test.go (right):

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/netchan_test.go#...
src/pkg/netchan/netchan_test.go:262: // Now hang up the channel.  Importer
should see it close.
I agree that the importer should see the channel close here,
but this is the opposite case: a Recv channel, not a Send one.

http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/netchan_test.go#...
src/pkg/netchan/netchan_test.go:299: // Now hang up the channel.  Importer
should see it close.
s/Importer/Exporter/

Same comment: closing the Recv side makes sense.

[r2, 2010-10-15 07:21:04]

On Oct 14, 2010, at 7:04 PM, rsc@google.com wrote:

> 
> http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/export.go
> File src/pkg/netchan/export.go (right):
> 
>
http://codereview.appspot.com/2469043/diff/1/src/pkg/netchan/export.go#newcod...
> src/pkg/netchan/export.go:351: // direction is Send, the channel is also
> closed. Messages in flight for the channel
> I think it would be simpler and just as good to cut the closing.
> It doesn't affect remote receivers, since they're on a different
> machine;
> if the local machine has no receivers, it's a no-op;
> and if the local machine has receivers, it's not clear you want to stop
> them.

i did that first, and it felt really odd to write
	exp.Hangup("name")
	close(channel)
and also, since the ids (name, chan) are in different spaces, potentially buggy.
 putting the close into Hangup resolved both issues.

you need to signal the remote end that no more data is coming if you're a
sender. what better mechanism than closing the channel, which already works?  i
honestly doubt anyone would desire to keep the local channel alive. it's
worthless locally, since you can't use it at all while the library's holding the
read end.  using it after you hangup sounds racy at best.
-rob

[rsc, 2010-10-18 21:02:53]

Maybe I have this backward, but I'm still confused.

If I do (following the doc comment):
    ch := make(chan int)
    imp.Import("receiver", ch, Recv)
    go func() { for v := range ch { fmt.Println(v) } }()

then I think it's completely fine for
    imp.Hangup("receiver")

to close(ch), causing the goroutine to stop receiving.
I think of the goroutine as owning the <-chan int side,
and I think of imp as owning the chan<- int side,
and close is a fine operation on the chan<- int side.

But if I do
    ch, done := make(chan int), make(chan bool)
    imp.Import("sender", ch, Send)
    go func() { for i := 0; i < 10; i++ { ch <- i }; done <- 1 }()
    <-done

then I think it's strange for
    imp.Hangup("sender")

because I think of the goroutine as owning
the chan<- int side, and I think of imp as owning
the <-chan int side, and close on the receive end
of a channel is typically a mistake and a race.

Russ

[r2, 2010-10-18 21:13:06]

On Oct 18, 2010, at 2:02 PM, Russ Cox wrote:

> Maybe I have this backward, but I'm still confused.
> 
> If I do (following the doc comment):
>    ch := make(chan int)
>    imp.Import("receiver", ch, Recv)
>    go func() { for v := range ch { fmt.Println(v) } }()
> 
> then I think it's completely fine for
>    imp.Hangup("receiver")
> 
> to close(ch), causing the goroutine to stop receiving.
> I think of the goroutine as owning the <-chan int side,
> and I think of imp as owning the chan<- int side,
> and close is a fine operation on the chan<- int side.
> 
> But if I do
>    ch, done := make(chan int), make(chan bool)
>    imp.Import("sender", ch, Send)
>    go func() { for i := 0; i < 10; i++ { ch <- i }; done <- 1 }()
>    <-done
> 
> then I think it's strange for
>    imp.Hangup("sender")
> 
> because I think of the goroutine as owning
> the chan<- int side, and I think of imp as owning
> the <-chan int side, and close on the receive end
> of a channel is typically a mistake and a race.

I see.  I guess it's a question of whom we think the Hangup is for.  I was
thinking of it from the point of view of the caller; you're thinking of it as
from the point of view of the library.  Either is plausible but both can't be
right.

You've half-convinced me I should flip the sense to that of the library but I'm
worried users will be confused.  Would documentation help?

-rob

[rsc, 2010-10-18 21:20:22]

On Mon, Oct 18, 2010 at 17:13, Rob 'Commander' Pike <r@google.com> wrote:
>
> On Oct 18, 2010, at 2:02 PM, Russ Cox wrote:
>
>> Maybe I have this backward, but I'm still confused.
>>
>> If I do (following the doc comment):
>>    ch := make(chan int)
>>    imp.Import("receiver", ch, Recv)
>>    go func() { for v := range ch { fmt.Println(v) } }()
>>
>> then I think it's completely fine for
>>    imp.Hangup("receiver")
>>
>> to close(ch), causing the goroutine to stop receiving.
>> I think of the goroutine as owning the <-chan int side,
>> and I think of imp as owning the chan<- int side,
>> and close is a fine operation on the chan<- int side.
>>
>> But if I do
>>    ch, done := make(chan int), make(chan bool)
>>    imp.Import("sender", ch, Send)
>>    go func() { for i := 0; i < 10; i++ { ch <- i }; done <- 1 }()
>>    <-done
>>
>> then I think it's strange for
>>    imp.Hangup("sender")
>>
>> because I think of the goroutine as owning
>> the chan<- int side, and I think of imp as owning
>> the <-chan int side, and close on the receive end
>> of a channel is typically a mistake and a race.
>
> I see.  I guess it's a question of whom we think the Hangup is for.
> I was thinking of it from the point of view of the caller;
> you're thinking of it as from the point of view of the library.
> Either is plausible but both can't be right.

Trying to think about it from the point of view of the caller...

If the caller imported/exported the channel for sending,
then I don't see much, if any, benefit to closing on Hangup.
(Close is a signal to receivers, and the receivers for this
channel are on a different machine.)
I believe the code closes ch in this case.

If the caller imported/exported the channel for receiving,
then it makes sense to close the channel on Hangup, so
that any receivers can, without any races, be sure that
no more data will be arriving.
I believe the code does not close ch in this case.

It could be just a matter of changing the close condition
from dir == Send to dir == Recv.

Russ

[r2, 2010-10-18 21:22:59]

On Oct 18, 2010, at 2:20 PM, Russ Cox wrote:

> On Mon, Oct 18, 2010 at 17:13, Rob 'Commander' Pike <r@google.com> wrote:
>> 
>> On Oct 18, 2010, at 2:02 PM, Russ Cox wrote:
>> 
>>> Maybe I have this backward, but I'm still confused.
>>> 
>>> If I do (following the doc comment):
>>>    ch := make(chan int)
>>>    imp.Import("receiver", ch, Recv)
>>>    go func() { for v := range ch { fmt.Println(v) } }()
>>> 
>>> then I think it's completely fine for
>>>    imp.Hangup("receiver")
>>> 
>>> to close(ch), causing the goroutine to stop receiving.
>>> I think of the goroutine as owning the <-chan int side,
>>> and I think of imp as owning the chan<- int side,
>>> and close is a fine operation on the chan<- int side.
>>> 
>>> But if I do
>>>    ch, done := make(chan int), make(chan bool)
>>>    imp.Import("sender", ch, Send)
>>>    go func() { for i := 0; i < 10; i++ { ch <- i }; done <- 1 }()
>>>    <-done
>>> 
>>> then I think it's strange for
>>>    imp.Hangup("sender")
>>> 
>>> because I think of the goroutine as owning
>>> the chan<- int side, and I think of imp as owning
>>> the <-chan int side, and close on the receive end
>>> of a channel is typically a mistake and a race.
>> 
>> I see.  I guess it's a question of whom we think the Hangup is for.
>>  I was thinking of it from the point of view of the caller;
>> you're thinking of it as from the point of view of the library.
>>  Either is plausible but both can't be right.
> 
> Trying to think about it from the point of view of the caller...
> 
> If the caller imported/exported the channel for sending,
> then I don't see much, if any, benefit to closing on Hangup.
> (Close is a signal to receivers, and the receivers for this
> channel are on a different machine.)
> I believe the code closes ch in this case.
> 
> If the caller imported/exported the channel for receiving,
> then it makes sense to close the channel on Hangup, so
> that any receivers can, without any races, be sure that
> no more data will be arriving.
> I believe the code does not close ch in this case.
> 
> It could be just a matter of changing the close condition
> from dir == Send to dir == Recv.

I looked at it from the same direction and got the opposite conclusion.  If
you're sending, you should close when you're done.  It's that simple.  Either
you're overthinking it or I'm underthinking it.

-rob

[rsc, 2010-10-18 21:29:59]

> I looked at it from the same direction and got the opposite conclusion.
> If you're sending, you should close when you're done.  It's that simple.
> Either you're overthinking it or I'm underthinking it.

Yes, you should close when you're done, so that the
receivers can see the close.  But in this case the
receivers are on another machine.  Closing on the
sending machine doesn't do anything.

Russ

[r2, 2010-10-18 21:33:07]

On Oct 18, 2010, at 2:29 PM, Russ Cox wrote:

>> I looked at it from the same direction and got the opposite conclusion.
>>  If you're sending, you should close when you're done.  It's that simple.
>> Either you're overthinking it or I'm underthinking it.
> 
> Yes, you should close when you're done, so that the
> receivers can see the close.  But in this case the
> receivers are on another machine.  Closing on the
> sending machine doesn't do anything.

But it does.  It causes the local side to tear down and, in turn, tell the other
side to tear down.  If I comment out the close call, both the import and export
tests hang.

-rob

[rsc, 2010-10-18 21:35:21]

> But it does.  It causes the local side to tear down and,
> in turn, tell the other side to tear down.  If I comment
> out the close call, both the import and export tests hang.

aha!  i missed that that's how Hangup got the teardown to run.
in that case i think it makes sense to close unconditionally:
either it helps the library (dir == Send) or it helps the user
(dir == Recv).

russ

[r, 2010-10-18 22:02:09]

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

Please take another look.

[rsc1, 2010-10-18 22:06:38]

LGTM

http://codereview.appspot.com/2469043/diff/13001/src/pkg/netchan/export.go
File src/pkg/netchan/export.go (right):

http://codereview.appspot.com/2469043/diff/13001/src/pkg/netchan/export.go#ne...
src/pkg/netchan/export.go:350: // Hangup disassociates the named channel from
the Exporter.  The channel is
and closes the channel?
(avoids passive)

[r, 2010-10-18 22:09:53]

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

netchan: add new method Hangup to terminate transmission on a channel
Fixes issue 1151.

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