proposal: Go 2: return error from close #30916

tandr · 2019-03-19T05:06:17Z

What version of Go are you using (`go version`)?

$ go version
go version go1.12.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

All of them, but I tested on darwin (macOS)

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tandr/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tandr/ww/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1n/xsv3_8z57h5bvld33wz_9crr0000gn/T/go-build587032202=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Trying to close() a previously closed channel (closed from another goroutine), or closing nil channel. Yes, I know currently it is illegal under penalty of panic() or total lockout on writing into bad channel.

https://play.golang.org/p/Iyy31orWyca

package main

var c chan int

func niller() {
	// checkpoint-0
	c = nil
}

func closer() {
	// checkpoint-1, unpredictable result
	close(c)

	// PROPOSAL: create an override of "close" that returns an error if code checks for it and
	// replace checkpoint-1 with
	// _ = close(c)
}

func main() {

	c = make(chan int, 1)

	go closer()
	go niller()

	// checkpoint-2
	if c != nil {
		closer()
		niller()
	}
}

What did you WANT to see?

This program to successfully finish always.

What did you see instead?

Sometimes it fails, sometimes it just normally finishes

Proposal

Add a compile-time override for close(channel) with signature close(channel) error that returns an error instead of calling panic on previously closed or nil channel. Define 2 errors - "channel closed" and "channel is nil" that this function can return instead of panic. Alternatively, it could be close(channel) bool, emulating the same behaviour that reading from the channel "value, stillOpen := <- ch"

example:

_ := close(ch) // will not panic if ch is nil, will not panic if ch was closed already

close(ch) // just like before, will behave as currently expected on closed or nil channel

Rationale

Using close() on channels is suggested as a method to notify reader side that channel is no longer usable, and/or job queue is exhausted. Problem is that even in examples (https://gobyexample.com/closing-channels) it is suggested to use a second channel to indicate "completion". Even if programmer have checked if channel is ready/not ready previously (by using select), it is still requires synchronization because it could be done only once if more than one writer exists.

Using fan-in or fan-out patterns of message aggregation/distribution, second channel is recommended as a way to identify "done", where if there was a way to safely notify "done" twice over the same channel, second "confirmation" channel would not be needed. After all, if reader went away, there is no need for the writer to continue - channel is stale anyway - but transmitting that information (channel no longer needed) requires a second channel or atomic bool.

I feel like proposal does not break Go1 Compatibility Guarantee - there is no code that checks return value of close() currently, so that code will continue to behave as it was originally designed.

Implementation

Compiler modifications are beyond my level of understanding of how Go compiler works.
Runtime modifications seems relatively not comlex by creating a second function similar to (or reusing) closechan(), (safe_closechan()?) in src/runtime/chan.go (https://golang.org/src/runtime/chan.go#L334) that does not panic in the first 2 checks.

Alternatives

`isClosed()`

Introduction of this function neither solves task completion notification problem, nor it will solve synchronization issues with calling close() even after isClose() returns true

Do not call `close()` unless you are absolutely need

As much as memleaks might not be a biggest problem, surprising lack of symmetry for "If something is opened, it needs to be closed" is a continuous stream of questions and confusion from developers.

Having additional backflow mechanism to synchronize "I confirm that it is done"

Not intuitive, and makes usage of channels harder.

Wrap `sync.Once` around close call, and use only that function to close the channel.

Workable solution, but encourages a code duplication for each type of the channel used, needs enforcing usage pattern "Use special Close() instead of direct close()", and still does not protect the cases when someone will call close on a channel directly. Also... adding an additional synchronization around what supposed to be a synchronization primitive feels (aesthetically?) wrong.

Conclusion

I think adding this override does not break Go1 Compatibility promise. It would make operations on channels be more consistent with "nil forgiveness" that makes working with maps and slices so enjoyable, and also would help with channels adoption. As I see right now by scouring libraries and Go own runtime and tools - channels are suggested to be used as an synchronization mechanism in Go, but adoption of it is extremely low.

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2019-03-19T14:54:16Z

Your example doesn't seem very illustrative, as there is no reason to call close there.

The only reason to ever call close is because the goroutine reading from the channel is checking whether the channel has been closed. I do not think that we should encourage people to call close for any other reason. That will increase confusion, not decrease it.

DeedleFake · 2019-03-19T15:11:39Z

Do not call close() unless you are absolutely need

As much as memleaks might not be a biggest problem, surprising lack of symmetry for "If something is opened, it needs to be closed" is a continuous stream of questions and confusion from developers.

I think this is more a consequence of unfortunate naming. Maybe something like finish(), stop(), or end() would have been better. It's a bit late to change it now, though.

Wrap sync.Once around close call, and use only that function to close the channel.

Workable solution, but encourages a code duplication for each type of the channel used, needs enforcing usage pattern "Use special Close() instead of direct close()", and still does not protect the cases when someone will call close on a channel directly. Also... adding an additional synchronization around what supposed to be a synchronization primitive feels (aesthetically?) wrong.

That's for sure. I've had quite a few times, especially when doing 1:M stuff with channels, where I've at least thought that I should wrap some of the channel accesses in mutexes or sync.Conds. Feels very weird. In the end that isn't always the best solution, but it sure seems to be the more obvious one, which kind of seems like a problem with the general flow of using channels for anything other than simple 1:1 communication. I would not at all be surprised to find that the majority of context.Context usage is simply to get access to the easy cancel() function that essentially does this for you. Seems a bit like NPM's infamous fascination with one-function libraries.

I don't think making close() return an error is necessarily the right solution for this. For one thing, it would be the only built-in function that would return a more complex type like that. It also feels odd having it essentially have a completely different functionality if and only if the user attempts to use the return value, even by assigning it to _.

tandr · 2019-03-19T16:23:10Z

@ianlancetaylor Thank you for the kind suggestion, I will try to provide a more elaborate example. I was trying to extract as little as possible from my past experience of dealing with it. Main points I am trying to convey are that

working with language level sync primitives (channels) should not require additional library level sync components (mutex/once), and
Consistency of the language is important and channels are a bit "off" of "the path of less surprise". On surface, they are behaving just like other language level primitives (use make to create it, assignable, nillable), but have surprises in regards to nil is treated (comparing to slices, maps).

@DeedleFake

Maybe something like finish(), stop(), or end() would have been better. It's a bit late to change it now, though.

Yes, I agree - that train have left the station long time ago. Naming it something like done, finish, or stop would be a better idea, but then it will probably cause numerous conflicts with an existing codebases where such a common names are heavily used already (our codebase uses stop()).

I would not at all be surprised to find that the majority of context.Context usage is simply to get access to the easy cancel() function that essentially does this for you.

Hehe, that's what I was thinking doing as well, but went a bit different direction due to unfamiliarity with Context back then. Used context for cancelation for a different part of the code later, when I finally wrapped my head about contexts a bit, but did not get to re-write of 1-N (fanout) messaging using this idea.

It also feels odd having it essentially have a completely different functionality if and only if the user attempts to use the return value, even by assigning it to _.

I think some seeds for that are there already - v := <-c vs v, ok := <-c, but overall I think you are right - it is a different behaviour based on a "health check". It is closer to how select with default behaves on channels with regards to "is it ready" - if not, choose the default without panic.

ianlancetaylor · 2019-03-19T16:36:09Z

I agree that in retrospect close is not the best name.

I don't agree that close is off the path of least surprise, once you understand the key point that close is a send operation that means "there will be no more sends on this channel." After calling close, any future send operation on the channel will panic. Since close itself a send operation, naturally calling close a second time will panic, just as any send operation will.

ianlancetaylor · 2019-03-26T21:28:38Z

We are not going to make this change.

tandr · 2019-03-27T04:04:50Z

Thank you Ian for the quick turnaround.

gopherbot added this to the Proposal milestone Mar 19, 2019

gopherbot added the Proposal label Mar 19, 2019

ianlancetaylor changed the title ~~Proposal: Have a safer close()~~ proposal: Go 2: return error from close Mar 19, 2019

ianlancetaylor added LanguageChange v2 A language change or incompatible library change labels Mar 19, 2019

ianlancetaylor closed this as completed Mar 26, 2019

golang locked and limited conversation to collaborators Mar 26, 2020

gopherbot added the FrozenDueToAge label Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: Go 2: return error from close #30916

proposal: Go 2: return error from close #30916

tandr commented Mar 19, 2019

ianlancetaylor commented Mar 19, 2019

DeedleFake commented Mar 19, 2019 •

edited

Do not call close() unless you are absolutely need

Wrap sync.Once around close call, and use only that function to close the channel.

tandr commented Mar 19, 2019 •

edited

ianlancetaylor commented Mar 19, 2019

ianlancetaylor commented Mar 26, 2019

tandr commented Mar 27, 2019

proposal: Go 2: return error from close #30916

proposal: Go 2: return error from close #30916

Comments

tandr commented Mar 19, 2019

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you WANT to see?

What did you see instead?

Proposal

Rationale

Implementation

Alternatives

isClosed()

Do not call close() unless you are absolutely need

Having additional backflow mechanism to synchronize "I confirm that it is done"

Wrap sync.Once around close call, and use only that function to close the channel.

Conclusion

ianlancetaylor commented Mar 19, 2019

DeedleFake commented Mar 19, 2019 • edited

Do not call close() unless you are absolutely need

Wrap sync.Once around close call, and use only that function to close the channel.

tandr commented Mar 19, 2019 • edited

ianlancetaylor commented Mar 19, 2019

ianlancetaylor commented Mar 26, 2019

tandr commented Mar 27, 2019

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?

`isClosed()`

Do not call `close()` unless you are absolutely need

Wrap `sync.Once` around close call, and use only that function to close the channel.

DeedleFake commented Mar 19, 2019 •

edited

tandr commented Mar 19, 2019 •

edited