Skip to content
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: runtime: select should panic sending to closed channels #19820

Closed
go101 opened this issue Apr 2, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@go101
Copy link

commented Apr 2, 2017

As this issue #11344 is locked, so I open this new one.

Now, sending to a closed channel in a select block only panics when the corresponding case is selected.
I propose that it should be always panic, whether such cases are selected or not , to make programs perform consistent.

I feel this doesn't break the compatibility.

@go101 go101 changed the title proposal: runtime: select should panic sending to closed channels proposal: runtime: select should panic sending to closed channels in select blocks Apr 2, 2017

@go101 go101 changed the title proposal: runtime: select should panic sending to closed channels in select blocks proposal: runtime: select should panic sending to closed channels Apr 2, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 2, 2017

Sorry, we've stopped making language changes for Go 1.x.

To write a formal language change proposal for consideration for any future Go 2.x, please write a design document. See https://github.com/golang/proposal#proposing-changes-to-go

I don't understand your proposal, though. It isn't specific enough. I would discuss further on the golang-nuts mailing list or Slack. See https://golang.org/wiki/Questions.

@go101

This comment has been minimized.

Copy link
Author

commented Apr 2, 2017

I don't understand your proposal, though

It is very simple, the following program (also shown in #11344) may panic, or not.

package main
func main() {
	a, b := make(chan int, 1), make(chan int, 1)
	close(a)
	select {
	case a<-1:
	case b<-1:
	}
}

or a simpler one:

package main
func main() {
	c := make(chan struct{})
	close(c)
	select {
	case c<-struct{}{}:
	case <-c:
	}
}

I don't think it is a good behavior, for either debug or test.
A good behavior should always panic for this program.
And I don't think this proposal is a language change.
In other words, I don't think it will break the go1 compatibility.

The document https://github.com/golang/proposal#proposing-changes-to-go doesn't require proposers must be write a formal design document. Also I think this proposal is so simple that it is not worth writing a formal one.

If you would like, you can tag it with a go2 label. :)

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 2, 2017

I'll see what others think.

@bradfitz bradfitz reopened this Apr 2, 2017

@bradfitz bradfitz added the Proposal label Apr 2, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2017

I kind of like the general idea. If something is going to panic with the roll of a die, might as well make it panic unconditionally.
It feels a bit like a special case, though. Should the following always panic? Certainly the answer is no. So what is different about these two examples?

   a, b := make(chan int, 1), make(chan int, 1)
   select {
      case a<-1: panic("foo")
      case b<-1:
   }

@bradfitz bradfitz added this to the Proposal milestone Apr 2, 2017

@go101

This comment has been minimized.

Copy link
Author

commented Apr 2, 2017

A whole select block can be viewed as a single operation + do something.
The difference is that, if a is closed, the single operation can be detected as dangerous for sure before performing channel reads/writes and doing something.

@go101

This comment has been minimized.

Copy link
Author

commented Apr 3, 2017

I suddenly realized that this change may make some obstacles for the future possible other improvements for select blocks: #18846 (comment),

This proposal requires all involved channels must be locked before further actions.
However, the possible improvements (ordered and stateful options) in #18846 (comment) don't require this.

So I decide to draw this proposal.

@go101 go101 closed this Apr 3, 2017

@go101 go101 reopened this Apr 3, 2017

@go101

This comment has been minimized.

Copy link
Author

commented Apr 3, 2017

reopen for this proposal may also not require all involved channels must be locked before further actions.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2017

This does not seem worth the complexity of a special case in select. I especially don't like that you can't short-circuit the select once you find a working case: you'd have to keep scanning to see if there are any panics. This makes the general code (for today's select) more complex and slower, for very little benefit. If you don't find out about the bug in this select, you'll hit it in the next one.

Also, priorities of any kind are antithetical to the point of select, which is to make a pseudo-random choice.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2017

Although this is on the face a very reasonable thing to require of select, the specification of select is very carefully written so it can be implemented efficiently and this proposal adds a large burden to the implementation that I feel it should not bear. Or to put it more concisely, this proposal constrains the implementation and makes select slower.

So with regret, closing.

@robpike robpike closed this Apr 3, 2017

@golang golang locked and limited conversation to collaborators Apr 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.