Closing a closed channel

[Alex Bligh]

Closing a closed channel causes a panic - I know that. I'm interested in why?

I've seen (and written) lots of code that does

func (x *X) Close() {
x.mutex.Lock()
defer x.mutex.Unlock()
if !x.closed {
x.closed = true
close(x.chan)
}
}

and have never seen a much simpler way to do it. A common reason is a
'done' signal on a chan struct{}. Yes I know you can use the context
package, but that surely does something similar under the hood.

Is there a good *reason* why closing a closed channel panics rather
than does nothing? e.g. does making it safe add a lot of overhead?

--
Alex Bligh

[Ian Lance Taylor]

Closing a channel is a signal to the goroutine reading from the
channel that there are no more values to send. There should never be
any confusion as to whether there are more values to send or not.
Either you are done, and you close the channel, or you are not done,
and you do not close the channel. Closing a channel twice implies
that you don't know whether you are done or not. By that argument,
closing a channel twice does not make sense, so the runtime panics.

It's true that there can be cases, especially when you aren't really
writing any values to the channel but are just using it as a
signalling mechanism, that you don't know whether you are done or not.
The current sense is that those cases are sufficiently rare that it's
appropriate to push the responsibility for the double close onto the
closer. Perhaps that should be rethought at some point, though not
for Go 1.7. The way to change this would be to make a proposal that
surveys some amount of Go code to see how much of it is (correctly)
checking for whether the channel has been closed before closing it.
If there is a lot of such code then perhaps the runtime should change
to silently ignore a close of a closed channel.

Ian

[atd...@gmail.com]

It's a sensible default because it's a mutation of shared state of the channel. From "not closed" to "being closed".

This default makes sense if you consider the whole picture of how concurrency works in Go.

That's also the reason there is no boolean value to check wether a channel has been closed because while returning, the channel might actually get clsoed.

The user needs to serialize access to channels in such cases.

[Alex Bligh]

On 27 Apr 2016, at 15:17, Ian Lance Taylor <ia...@golang.org> wrote:

> Closing a channel is a signal to the goroutine reading from the
> channel that there are no more values to send. There should never be
> any confusion as to whether there are more values to send or not.
> Either you are done, and you close the channel, or you are not done,
> and you do not close the channel. Closing a channel twice implies
> that you don't know whether you are done or not. By that argument,
> closing a channel twice does not make sense, so the runtime panics.
>
> It's true that there can be cases, especially when you aren't really
> writing any values to the channel but are just using it as a
> signalling mechanism, that you don't know whether you are done or not.
> The current sense is that those cases are sufficiently rare that it's
> appropriate to push the responsibility for the double close onto the
> closer. Perhaps that should be rethought at some point, though not
> for Go 1.7.

In the majority of non-broken case of the mutex-protecting-close,
it is (or should be) a chan struct{}, and nothing is/was ever sent.

I think one way of putting what you wrote is "writers should do the
closing, and more than one writer without synchronisation of closing
is going to lead inevitably to a TOCTOU problem where a writer
does not expect the channel to be closed". That is not the case
where nothing is sent (i.e. where there is no writer); I'm having
difficulty thinking of non-obscure examples where there is a writer.

> The way to change this would be to make a proposal that
> surveys some amount of Go code to see how much of it is (correctly)
> checking for whether the channel has been closed before closing it.
> If there is a lot of such code then perhaps the runtime should change
> to silently ignore a close of a closed channel.

I would have no idea how to set about doing that.

--
Alex Bligh

[Ian Lance Taylor]

If I were going to do this, which I am not, I would look on github for
the 100 most popular Go packages, fetch them with go get, grep them
for calls to close, and check which percentage of those calls have an
obvious check to avoid a double close.

Ian

[Axel Wagner]

On Wed, Apr 27, 2016 at 3:52 PM, Alex Bligh <al...@alex.org.uk> wrote:
> and have never seen a much simpler way to do it.

func Close(ch chan int) {
defer func() { recover() }()
close(ch)
}

works fine, as does (and you should probably prefer this to your variation):

type X struct {
o sync.Once
ch chan int
}

func (x *X) Close() {
x.o.Do(func() { close(x.ch) })
}

[Jakub Labath]

Indeed sync.Once is the simpler and nicer way to do this.

Also nitpick why do this

if !x.closed {
        x.closed = true
        close(x.chan)
}

when x.closed could be a chan such as chan struct{}.

any other code trying to establish if it object is finished then only needs to do

select {
  case <-x.closed:
      //closed
   default:
      //open
}

I use this frequently e.g. I can launch a cleanup go routine at the time of object creation, that simply waits on that x.closed. I found that cleaning up at the same time I'm creating is quite helpful and clean. Of course that may not be the best thing to do if you have millions of such objects. Anyway I found using a closed (done) channel like that gives me way more flexibility than checking some boolean value where I indeed need to worry about synchronizing access.

Cheers

[Alex Bligh]

Jakub,

On 28 Apr 2016, at 15:03, Jakub Labath <jla...@labath.ca> wrote:

> Indeed sync.Once is the simpler and nicer way to do this.

Yes that's interesting.

> Also nitpick why do this
>
> if !x.closed {
> x.closed = true
> close(x.chan)
> }

closed is only protecting closing the channel more than once, and ...

> when x.closed could be a chan such as chan struct{}.
>
> any other code trying to establish if it object is finished then only needs to do
>
> select {
> case <-x.closed:
> //closed
> default:
> //open
> }

... the whole idea is that you can do exactly that with the structure as presented.

Alex

> I use this frequently e.g. I can launch a cleanup go routine at the time of object creation, that simply waits on that x.closed. I found that cleaning up at the same time I'm creating is quite helpful and clean. Of course that may not be the best thing to do if you have millions of such objects. Anyway I found using a closed (done) channel like that gives me way more flexibility than checking some boolean value where I indeed need to worry about synchronizing access.
>
> Cheers
>
> On Wednesday, April 27, 2016 at 10:34:10 PM UTC, Axel Wagner wrote:
> On Wed, Apr 27, 2016 at 3:52 PM, Alex Bligh <al...@alex.org.uk> wrote:
> > and have never seen a much simpler way to do it.
>
> func Close(ch chan int) {
> defer func() { recover() }()
> close(ch)
> }
>
> works fine, as does (and you should probably prefer this to your variation):
>
> type X struct {
> o sync.Once
> ch chan int
> }
>
> func (x *X) Close() {
> x.o.Do(func() { close(x.ch) })
> }
>

> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Alex Bligh

[Jakub Labath]

... the whole idea is that you can do exactly that with the structure as presented.

Alex

 

Right for that sync.Once is your best friend. The point I was trying to make done channels work great because it does anything a bool variable does but also comes automatically synchronized. And as such can be waited upon if desirable by any other part of your code.
If all you are doing is closing that one other channel in that struct it is obviously overkill.

Cheers

[Andrey Tcherepanov]

Ian,

Sorry to revive this old thread. Would it be possible to add your answer (and a solution with sync.Once suggested on this thread) to a FAQ and/or Effective Go document?

Yes, I stepped on it too trying to use close() as a signal that work is done, and then doing close() in a cleanup goroutine :)

Thank you very much,

   Andrey