New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Go 2: sends on closed channels do not panic #21985

Closed
jeromefroe opened this Issue Sep 23, 2017 · 21 comments

Comments

Projects
None yet
8 participants
@jeromefroe

jeromefroe commented Sep 23, 2017

Motivation

A common pattern I've seen is using a lock to protect a channel and a variable which indicates whether or not the channel is closed. For example:

type Queue struct {
  sync.RWMutex

  closed bool
  ch chan T
}

func New(cap int) *Queue {
  return &Queue{ ch: make(chan T, cap) } 
}

func (q *Queue) Enqueue(t T) error {
  q.RLock()
  defer q.RUnlock()
  if q.closed {
    return errors.New("queue is closed")
  }
  q.ch <- t
}

func (q *Queue) Close() {
  q.Lock()
  defer q.Unlock()
  if q.closed {
    return
  }
  close(q.closed)
  q.closed = true
}

Using this approach one has to acquire two locks in each call to Enqueue. The first lock being the read lock for q.closed and the second being the internal lock used by the channel. However, the channel already has a field closed to indicate whether the channel has been closed and a lock which protects it, so the extra lock is only necessary because there is no publicly accessible API for determining if the channel is closed.


Proposal

To support cases like the one above, where a channel may be closed concurrently with sends on it, I'd like to propose that Go add support for conditionally sending values on a channel by returning a bool from send operations to indicate whether the send was successful. Sends to a channel could then do the following:

 if ok := q.ch <- t; !ok {
  return errChanClosed
}

Callers that do, in fact, want the send to panic could explicitly call panic themselves. While those that want to treat such a case as an error will have that ability as well.

Thanks!

@gopherbot gopherbot added this to the Proposal milestone Sep 23, 2017

@gopherbot gopherbot added the Proposal label Sep 23, 2017

@robpike

This comment has been minimized.

Contributor

robpike commented Sep 23, 2017

Under this proposal, incorrect code that broke before will silently fail, that is, appear to run correctly while still being incorrect. That seems retrograde.

@go101

This comment has been minimized.

go101 commented Sep 23, 2017

how about just like type assertion, if ok is present, then not panic, otherwise, panic just like Go 1.

@robpike

This comment has been minimized.

Contributor

robpike commented Sep 23, 2017

If a program sends on a closed channel, it's not managing resources correctly. You could argue that your proposal makes it easier to do that, but I would counter that it makes it too easy to construct poor concurrent structures.

@cznic

This comment has been minimized.

Contributor

cznic commented Sep 23, 2017

Also, let's make integer division by zero silently return a random number. /s

@jeromefroe

This comment has been minimized.

jeromefroe commented Sep 23, 2017

Under this proposal, incorrect code that broke before will silently fail, that is, appear to run correctly while still being incorrect. That seems retrograde.

I think there are a few approaches that could be considered in order to address this issue. The first is that static analysis tools could be used to highlight to users where in their code they are not checking the return value of the send. There's a precedence for this already with linters such as errcheck that find unchecked errors. A second, more heavy-handed approach, would be for the compiler to reject any code that does not check the return value of a send, so that ch <- t becomes illegal. Then callers would be forced to check the return value or at the very least explicitly throw it away.

how about just like type assertion, if ok is present, then not panic, otherwise, panic just like Go 1.

My only hesitation with this approach is that I can't think of any other places in the language where _ = <operation> and <operation> have different semantics when the operation has a single return value. For example, in the case of a type assertion _ = t.(type) and t.(type) have the same semantics the latter form is invalid. I'm not saying I'm personally opposed to this idea though as it would address the issue above of old code now silently failing, just that I'm not sure if the broader community would find it appealing given that it introduces an arbitrary exception.

If a program sends on a closed channel, it's not managing resources correctly. You could argue that your proposal makes it easier to do that, but I would counter that it makes it too easy to construct poor concurrent structures.

I'm not sure that I agree that it would make it much easier to construct poor concurrent structures given that it's already possible to emulate this behavior with an additional lock around the channel, as the Queue example above shows. Since channels are often a point of synchronization, I think it's natural for them to serve as a signal of events in the lifecycle of a program. Allowing sends to fail gracefully would then allow users to propagate information about resources being closed.

@cznic

This comment has been minimized.

Contributor

cznic commented Sep 23, 2017

My only hesitation with this approach is that I can't think of any other places in the language where _ = and have different semantics.

_ = f() != f() when f has any other number of return values than 1, eg. as in func f() {}.

For example, in the case of a type assertion _ = t.(type) and t.(type) have the same semantics.

The later form is invalid.

@jeromefroe

This comment has been minimized.

jeromefroe commented Sep 23, 2017

_ = f() != f() when f has any other number of return values than 1, eg. as in func f() {}

My apologies, I should have been more explicit, I was referring to function calls that have a single return value. Updated my comment to reflect that.

The later form is invalid.

You're right! Thanks! I had blindly assumed it would work the same as a function call. Updated my comment appropriately.

@jech

This comment has been minimized.

jech commented Sep 23, 2017

Here's an actual example where this feature would be useful.

I've got some code where a set of goroutines are managing data that is made available over an HTTP interface. When the HTTP code receives a request, if finds a channel to the relevant goroutine in a global sync.Map, then sends a request for the data to the channel it found.

When data is deleted, the goroutine that manages it first removes the channel from the sync.Map, then closes the channel, and then dies. Hence, the channel found by the HTTP code is usually open. However, there's a race — if the channel is closed between the point when it is fetched and the point where it is written to, then the HTTP goroutine panics.

I don't see any clean way to solve this without locking. Currently, I don't bother, and live with the occasional panic.

@go101

This comment has been minimized.

go101 commented Sep 23, 2017

@jech you don't need to close the channels, you can use an extra channel (called it exited) for each element in the sync.Map. You can close the exited instead and then you can use a select block to send data, though a select block is not as efficient as a single send operation.

Here are some examples: http://www.tapirgames.com/blog/golang-channel-closing

@jech

This comment has been minimized.

jech commented Sep 23, 2017

you can use an extra channel

Sure, I can make the structure of my code increasingly complex. Or I could simply write

ch, ok <- &Request{...}
if !ok {
    return 0, ErrDataUnavailable
}

in the reader passed to http.ServeContents.

@robpike

This comment has been minimized.

Contributor

robpike commented Sep 24, 2017

As others have pointed out, the language already has all the tools you need to achieve your goal, and despite your protests it's really not much more complex to use them.

And as I said before, your proposal makes it too easy to write bad concurrent structures. It's analogous to how reentrant locks result in bad locking protocols.

@jech

This comment has been minimized.

jech commented Sep 24, 2017

@go101

This comment has been minimized.

go101 commented Sep 24, 2017

Just realized that, for send operations as case conditions, this proposal makes it is hard to keep compatibility. For solo send operations, it is not a big problem.

Good or bad, whether the ok is present will also make effect on the behaviors of select blocks.

@jeromefroe

This comment has been minimized.

jeromefroe commented Sep 24, 2017

And as I said before, your proposal makes it too easy to write bad concurrent structures. It's analogous to how reentrant locks result in bad locking protocols.

Forgive me if I'm beating a dead horse, but it's not clear to me why this proposal makes it too easy to write bad concurrent structures. Admittedly though, I don't have much experience with reentrant locks, so I didn't quite see how the analogy applies.

My thought with this proposal was that since channels serve as synchronization points, one can use a closed channel as a way to cancel in-flight goroutines. For example, a program may have a source of goroutines (a server which creates a new goroutine for each connection being one example) that perform some processing and then put a value on a channel. When an event occurs the program may want to close the source, so no new goroutines are created, and the channel as well, so that any in-flight goroutines are cleaned up.

@jech

This comment has been minimized.

jech commented Sep 25, 2017

I wrote previously:

ch, ok <- &Request{...}
if !ok {
    return 0, ErrDataUnavailable
}

Looking at this some more, there's a race condition: if the (asynchronous) channel is closed just after the request is written, the request is discarded with no error indication to the caller.

So I'm slowly starting to feel that @robpike is right — this proposal would lead to bad code.

@jeromefroe

This comment has been minimized.

jeromefroe commented Sep 25, 2017

Looking at this some more, there's a race condition: if the (asynchronous) channel is closed just after the request is written, the request is discarded with no error indication to the caller.

Wouldn't that be equivalent to closing a buffered channel that has values in it though? The values are still in the channel and available to be pulled off.

@gopherbot

This comment has been minimized.

gopherbot commented Jan 6, 2018

Change https://golang.org/cl/86515 mentions this issue: sync: add Chan, in order to avoid panic when send on closed channel

@gopherbot

This comment has been minimized.

gopherbot commented Jan 6, 2018

Change https://golang.org/cl/86555 mentions this issue: sync: add Chan, in order to avoid panic when send on closed channel

@gopherbot

This comment has been minimized.

gopherbot commented Jan 6, 2018

Change https://golang.org/cl/86555 mentions this issue: sync: add syncchan, in order to avoid panic when send on closed channel

@bronze1man

This comment has been minimized.

bronze1man commented Feb 1, 2018

I just found another problem to this "sends on closed channels panic" problem when working with "M receivers, N senders and X closers channel" problem.

"close the channel" and "send message to it with panic recover" in two different goroutines is a data race condition to the golang current data race detecter.
So "send message to it with panic recover" workaroud is not equal to "close an additional signal channel" workaroud.

And the "close an additional signal channel" workaroud is difficult to work right when I need read all the data from the channel after the channel closed.

So if I can use sync.Mutex to solve my problem,I will not use channel. It is more complex then I first thought. The workaround https://golang.org/cl/86555 looks good to me.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 21, 2018

There are good arguments against this proposal above, and few supporters. Declining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment