runtime: close of a channel with pending sends incorrectly flagged by race detector

[kjk]

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

$ go version
go version go1.11.4 windows/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output

$ go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\kjk\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\kjk\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\kjk\AppData\Local\Temp\go-build206244702=/tmp/go-build -gno-record-gcc-switches

What did you do?

Save this to e.g. t.go:

package main

import (
	"fmt"
	"time"
)

func main() {
	ch := make(chan string)
	for i := 0; i < 256; i++ {
		go func(n int) {
			ch <- fmt.Sprintf("string %d", n)
		}(i)
	}
	time.Sleep(time.Second)
	close(ch)
}

and run go -race t.go.

What did you see instead?

I see a data race reported:

> go run -race t.go
==================                                                                
WARNING: DATA RACE                                                                
Write at 0x00c00001a130 by main goroutine:                                        
  runtime.closechan()                                                             
      C:/Go/src/runtime/chan.go:327 +0x0                                          
  main.main()                                                                     
      C:/Users/kjk/src/t.go:16 +0xb5                                              
                                                                                  
Previous read at 0x00c00001a130 by goroutine 231:                                 
  runtime.chansend()                                                              
      C:/Go/src/runtime/chan.go:140 +0x0                                          
  main.main.func1()                                                               
      C:/Users/kjk/src/t.go:12 +0xcc                                              
                                                                                  
Goroutine 231 (running) created at:                                               
  main.main()                                                                     
      C:/Users/kjk/src/t.go:11 +0x83                                              
==================                                                                
panic: send on closed channel                                                     
                                                                                  
goroutine 201 [running]:                                                          
main.main.func1(0xc00001a120, 0xc3)                                               
        C:/Users/kjk/src/t.go:12 +0xcd                                            
created by main.main                                                              
        C:/Users/kjk/src/t.go:11 +0x84                                            
panic: send on closed channel                                                     
                                                                                  
goroutine 235 [running]:                                                          
main.main.func1(0xc00001a120, 0xe5)                                               
        C:/Users/kjk/src/t.go:12 +0xcd                                            
created by main.main                                                              
        C:/Users/kjk/src/t.go:11 +0x84                                            
exit status 2                                                                     

What did you expect to see?

I didn't expect data race between close(ch) and pending ch <- v send.

The spec (https://golang.org/ref/spec#Channel_types) is curiously mum on the subject.

It says:

A single channel may be used in send statements, receive operations, and calls to the built-in functions cap and len by any number of goroutines without further synchronization.

By not mentioning close it implies that one has to synchronize close and sends.

But I wouldn't know how to synchronize those. My sends have already been dispatched and closing a channel seems like a valid (and only) way to ensure those sending goroutines don't stick forever (assumingI recover the resulting panic).

The data race seems harmless, as I'm done with this channel, but I run my tests with -race detection enabled and this makes the test fail.

[zigo101]

Similar to #27769

[as]

It's a valid transaction race. There is no guarantee the sends complete before the close, which would result in a panic. The race detector actually helped you here.

[kjk]

@as The code with data race is in Go's runtime, not in my code.

Where does the spec declare this code as generating a data race?

As stated in my bug report, I understand that this panics as that is what the spec says and I'm fine with that.

I'm not fine with this also being reported as a data race as the spec doesn't say that closing a channel with pending sends is undefined behavior (which would be enough of an escape to consider the implementation conforming).

[odeke-em]

Thank you for this report @kjk!

In deed as @go101 noted, it is a duplicate of #27769 which is still open and trying to decide how to document this. As @as has noted it is a valid transaction race but as you too have found there is no documentation in the spec about this. Let's move this to that issue as it is also actionable but more discussion has gone on there. I'll close this one in favor of #27769 but please feel free to reopen this one.

[kjk]

I don't have permission to re-open the bug, @odeke-em could you please re-open it.

This is not a dup #27769.

The discussion in that bug operates under the assumption that this:

	ch := make(chan string)
	for i := 0; i < 256; i++ {
		go func(n int) {
			ch <- fmt.Sprintf("string %d", n)
		}(i)
	}
	close(ch)

is invalid Go program, that race detector is rightfully detecting a data race here and it's just a matter of documenting that.

I've read the discussion and I didn't find a justification for that position.

The fact that close of a channel with pending sends is considered invalid Go code would certainly violate my assumptions, given that raison d'etre for channels to allow thread-safe exchange of data between goroutines.

I claim that the above is a valid Go code and there's a bug in Go's implementation of close/send or implementation of race detector.

Maybe I'm wrong but I would like someone involved in the spec/compiler/race detector to opine on that. Without that closing this bug seems premature.

[kjk]

Furthermore, #27070 describes a similar race between close and len/cap which was deemed a bug of enough severity that the fix was backported to 1.11.

[kjk]

I can't quite match up the line numbers in chan.go but chansend() is reading c.closed outside of c.lock while closechan() is writing to it inside c.lock.

That suggests the intent was read/writes of c.closed to be protected and chansend() failing to do so is a bug that would manifest as a data race.

Furthermore chanrecv() uses atomic.Load() to read c.closed further muddying the water as to how the variable is supposed to be used.

Presumably always using atomic operations for c.closed would be the right thing to do.

[as]

There should be two checks. One fast path outside the critical section and another one inside. I'm not sure where there's suddenly an atomic being used in chanrecv. This code in particular always seemed to avoid atomics for checking closed.

[ALTree]

Reopening this since @odeke-em said "feel free to reopen this one" and the issue's author has requested it.