code review 5491059: time: close Timer/Ticker channel on Stop()

time: close Timer/Ticker channel on Stop()

[vadik, 2011-12-16 04:59:01]

Hello golang-dev@googlegroups.com,

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

[vadik, 2011-12-16 04:59:24]

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

Please take another look.

[jmaahir, 2011-12-16 05:09:47]

jmaahir@hotmail.com

[iant, 2011-12-16 05:59:22]

http://codereview.appspot.com/5491059/diff/4001/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):

http://codereview.appspot.com/5491059/diff/4001/src/pkg/time/sleep.go#newcode41
src/pkg/time/sleep.go:41: if c, ok2 := t.r.arg.(chan Time); ok2 {
I think you can just write
    close(t.r.arg.(chan Time))
Nobody should be fiddling with the C field.

[rsc, 2011-12-16 06:01:03]

This came up once before and we decided not to do this.
The problem is that closing the channel makes any pending
receive unblock and deliver a zero value, which could cause
significant confusion in existing code.  In general channels
do not have to be closed (they are not like file descriptors),
and there is no real reason to close this one.  In particular
there are no range loops involved.

Russ

[dvyukov, 2011-12-16 09:16:40]

On Fri, Dec 16, 2011 at 10:01 AM, Russ Cox <rsc@golang.org> wrote:
> This came up once before and we decided not to do this.
> The problem is that closing the channel makes any pending
> receive unblock and deliver a zero value, which could cause
> significant confusion in existing code.  In general channels
> do not have to be closed (they are not like file descriptors),
> and there is no real reason to close this one.  In particular
> there are no range loops involved.

Range with Ticker looks reasonable IMHO

[vadik, 2011-12-16 09:24:41]

On Dec 16, 2011 7:01 AM, "Russ Cox" <rsc@golang.org> wrote:
>
> This came up once before and we decided not to do this.
> The problem is that closing the channel makes any pending
> receive unblock and deliver a zero value, which could cause
> significant confusion in existing code.

That was my feeling after looking at it for some time, but golang-nuts made
me do it :)

Closing the issue?

Vadik.

[rsc, 2011-12-16 15:13:24]

On Fri, Dec 16, 2011 at 04:16, Dmitry Vyukov <dvyukov@google.com> wrote:
> Range with Ticker looks reasonable IMHO

Sorry.  Closing the chan would have been fine at the start.
It is too subtle a change to make now.

Russ

[rsc, 2011-12-16 15:13:33]

On Fri, Dec 16, 2011 at 04:24, Vadim Vygonets <unixdj@gmail.com> wrote:
> Closing the issue?

Sounds good to me.

[albert.strasheim, 2012-01-09 16:52:52]

Hello

On 2011/12/16 15:13:33, rsc wrote:
> On Fri, Dec 16, 2011 at 04:24, Vadim Vygonets <mailto:unixdj@gmail.com> wrote:
> > Closing the issue?
> 
> Sounds good to me.

One last suggestion (feel free to reject it): what about a StopClose function?

Cheers

Albert

[rsc, 2012-01-09 18:56:52]

Let's leave the API as it is for now.