Skip to content

proposal: Go2: allow send on a closed channel to return status on select #28017

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

Closed
OneOfOne opened this issue Oct 4, 2018 · 12 comments
Closed
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Milestone

Comments

@OneOfOne
Copy link
Contributor

OneOfOne commented Oct 4, 2018

I have proposed this before (#15411), but at the time Go2 wasn't anywhere near planned.

The problem

  1. In multi-writers/readers situations, the user has to use an extra channel or context.Context.
  2. Users either implement their minimal version of Context or use Context with the slight overhead just as a safe channel barrier.

The proposal

  1. Allow close(ch) to always succeed.
  2. Allow:
select {
   case ok := ch <- val:
      if !ok { exit logic }
   case ch <- val: // this still panics if the channel is closed.
}

Go 1 compat

The the 2nd part of the proposal is 100% compatible with Go1, it will not break any current code since the syntax wouldn't be used in any older projects, the 1st part however can be iffy if you have tools that depend on the panic (I don't know any)

This also can also solve #27982.

@gopherbot gopherbot added this to the Proposal milestone Oct 4, 2018
@OneOfOne
Copy link
Contributor Author

OneOfOne commented Oct 4, 2018

@gopherbot please add go2.

@gopherbot gopherbot added the v2 An incompatible library change label Oct 4, 2018
@DeedleFake
Copy link

DeedleFake commented Oct 4, 2018

I used to be in favor of something like this but I've become a lot more ambivalent about it more recently. The thing is that you actually usually don't want to have uncoordinated channel closing from multiple writers. With these changes, the first writer to finish will close the channel and all of the other writers will just fail to write any more data they may have afterwards. This isn't ideal in most situations, and for the ones where it is you can always just use a sync.WaitGroup and a second channel to coordinate better.

I think a lot of these channel issues that people have come down to the annoyance of having to create types just to be able to cleanly pass multiple pieces of data around easily in some instances, such as through channels. In this case, the annoyance stems from having to pass not only a single channel but an extra chan struct{} and a sync.WaitGroup/sync.Once around between all of the writers, and also passing both of those channels to the readers. This can get kind of awkward in some cases, especially since creating a new type just to deal with that specific situation that's probably only ever going to be in that one place in the code sometimes feels like overkill. It also can lead to some rather nasty APIs when those channels need to be used manually by the user. Wrapping those in a function call can help, but then it doesn't work with select and other channels.

I think one thing that could help, at least for internal usage, is some type of tuple, such as the pseudo-tuples that would be provided by #12854. It seems sadly unlikely at this point, though. I'm not so sure how to fix the API issue, but generics might help depending on how they handle operators, or maybe just through a chans package or something.

@bradfitz
Copy link
Contributor

bradfitz commented Oct 4, 2018

I have proposed this before (#15411), but at the time Go2 wasn't anywhere near planned.

So what's different this time?

@zigo101
Copy link

zigo101 commented Oct 4, 2018

The second one is same as #21985 ?

edit: I think that one is better than this one.

@OneOfOne
Copy link
Contributor Author

OneOfOne commented Oct 4, 2018

@bradfitz absolutely nothing, except now we have the Context package which a big chunk of its usage is a workaround not being able to easily sync on one channel.

@deanveloper
Copy link

I think that similar to one of my previous issues #27544, while it has uses, it encourages bad habits, and the boilerplate required to substitute isn't really that much (an extra <-chan struct{} that'd get passed to your readers)

@OneOfOne
Copy link
Contributor Author

OneOfOne commented Oct 4, 2018

@go101 actually yes, I just forgot about that, except my proposal only works in a select.

@deanveloper it's not that simple most of the time, that's a big part of the reason we have the context package to begin with, to work around this problem (passing values is of course important but that's irrelevant to this proposal).

@ianlancetaylor ianlancetaylor added the LanguageChange Suggested changes to the Go language label Oct 4, 2018
@as
Copy link
Contributor

as commented Oct 4, 2018

Users either implement their minimal version of Context or use Context with the slight overhead just as a safe channel barrier.

I don't understand what you mean here. What is a channel barrier? Why do you need context to achieve this?

@pwaller
Copy link
Contributor

pwaller commented Oct 5, 2018

This proposal as it stands doesn't meet the high bar I would expect for changing the rules of the language.

I hope this doesn't seem dismissive. I've spent some rare free time to write my opinion below in the hopes that it helps in the refinement of any proposal you might want to make. I'm not a member of the Go team, but an independent bystander.

I'm not persuaded there is a problem. If the language would benefit from this change, it isn't shown how in a clear way. This could be done with an experience report and some code, for example showing the old way, how the new way would work, and at least a short discussion about what the implications are for correctness when it comes to typical developers trying to write programs.

Members of the Go team have explained that the current semantics are an intentional design choice intended to help find bugs. I believe those choices have helped me find bugs and learn how to use these primitives correctly in a concurrent arrangement. This is a difficult task in general.

One property of the way it currently works that I think we all benefit from is that if you bump up against this ("panic writing on closed channel"), there is one clear way to proceed: don't write on a closed channel. It isn't allowed. Compose other primitives to avoid doing it. The rules are clear.

The lack of choice makes the decision of how to proceed very easy. It requires you to do design work, so proceeding itself might not be easy, but at least you're more likely to end up with a correct system for the additional thought you put in.

With this proposal, there are now two options: 1) don't do that, 2) try to do it and it might fail, handle failure at runtime. As I think has been said elsewhere, the second option introduced by this proposal is not clearly a benefit: it could easily lead to silently lost data and a tricky to track down bug.

@OneOfOne
Copy link
Contributor Author

OneOfOne commented Oct 5, 2018

@pwaller thanks for the well written response.

My proposal won't change anything regarding the write on closed channel unless you specifically use case ok := ch <- val, without the ok := part it'd still panic like it currently does.

There's no lack of choice really, it's pretty much like checking for errors returned from a func vs ignoring them, if you ignore the error.

And the use case for that is simply avoiding this from the context package:

func propagateCancel(parent Context, child canceler) {
	if parent.Done() == nil {
		return // parent is never canceled
	}
	if p, ok := parentCancelCtx(parent); ok {
		p.mu.Lock()
		if p.err != nil {
			// parent has already been canceled
			child.cancel(false, p.err)
		} else {
			if p.children == nil {
				p.children = make(map[canceler]struct{})
			}
			p.children[child] = struct{}{}
		}
		p.mu.Unlock()
	} else {
		go func() {
			select {
			case <-parent.Done():
				child.cancel(false, parent.Err())
			case <-child.Done():
			}
		}()
	}
}

I completely understand the wording of my proposal might not be very clear, but I'm not really sure how to improve it, I will try to add some code examples later tonight.

@pwaller
Copy link
Contributor

pwaller commented Oct 6, 2018

And the use case for that is simply avoiding this from the context package:

I don't see how it follows that your proposal will obviate the need for propagateCancel. Among other things, the code you linked to propagates error values, but I don't see how your proposal would achieve an equivalent thing. I also don't see what's wrong with propagateCancel or why one would want to avoid it. It can be used to solve a problem. Is there a specific issue with it?

'avoiding this from the context package' doesn't really fit within the spirit of a "use case":

A description of a potential scenario in which a system receives an external request (such as user input) and responds to it.

The important thing in my mind is to try and 1) articulate a problem you're trying to solve, 2) illustrate how you'd solve it with the current semantics, and then 3) illustrate how you'd fix it with the new semantics, and 4) try and make it clear why that's better.

"Avoiding using some code" doesn't shed light on what the underlying issues are. This is important because alternatives should be considered which don't involve changing the language semantics.

I realise that you don't think changing the language semantics is a big deal in this case, since it can be done in a way which doesn't change the behaviour of any existing code. I would argue that there is still a significant impact at least in that all experienced users of the language now have to learn this new semantic. It opens up new architectures involving channels.

Even if you don't use the feature because you're unaware of it, if you're reading someone else's code, they might use that. I would find it surprising if this did change.

@ianlancetaylor
Copy link
Contributor

Closing a channel is a send operation on the channel, and it needs to coordinate with other send operations. You are suggesting a mechanism that allows one channel send to coordinate with another channel send in a very specific case, specific to closing the channel. But it doesn't address the general case of coordinating sends. Also you seem to be suggesting that this is only handled in select, which is another very specific case. Sorry, but we're not going to do this.

@golang golang locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

9 participants