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.

Sign up for GitHub

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

Jump to bottom

runtime/race: false positive with closed channels in default select (go1.14beta1) #36714

Closed
zeebo opened this issue Jan 23, 2020 · 8 comments
Closed

runtime/race: false positive with closed channels in default select (go1.14beta1) #36714

zeebo opened this issue Jan 23, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.14

Comments

@zeebo
Copy link
Contributor

zeebo commented Jan 23, 2020

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

go1.14beta1

$ go version
go version devel +a5bfd9da1d Tue Dec 17 14:59:30 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No, go1.13.6 does not have the problem.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jeff/.cache/go-build"
GOENV="/home/jeff/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jeff/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org"
GOROOT="/home/jeff/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/jeff/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jeff/tmp/raceissue/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build491500605=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

package main

func main() {
	for {
		var loc int
		var write = make(chan struct{})
		var read = make(chan struct{})

		go func() {
			select {
			case <-write:
				_ = loc
			default:
			}
			close(read)
		}()

		go func() {
			loc = 1
			close(write)
		}()

		<-read
	}
}

I ran the above code with GORACE="halt_on_error=1" go run -race *.go

What did you expect to see?

No error.

What did you see instead?

==================
WARNING: DATA RACE
Read at 0x00c000112000 by goroutine 6:
  main.main.func1()
      /home/jeff/tmp/raceissue/race.go:12 +0x73

Previous write at 0x00c000112000 by goroutine 7:
  main.main.func2()
      /home/jeff/tmp/raceissue/race.go:19 +0x38

Goroutine 6 (running) created at:
  main.main()
      /home/jeff/tmp/raceissue/race.go:9 +0xd4

Goroutine 7 (finished) created at:
  main.main()
      /home/jeff/tmp/raceissue/race.go:18 +0x100
==================
exit status 66

I did a git bisect between go1.13.6 and go1.14beta1 and it pointed at e1446d9 which seemed to be a great likely candidate. After finding that, I found that the issue does not present itself if the write channel in the example is sent on instead of closed, or if there is not a default case in the select. Additionally, running on go1.14beta1 with that commit reverted also causes the example to succeed.
@toothrot toothrot added this to the Backlog milestone Jan 23, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 23, 2020
@toothrot
Copy link
Contributor

Thanks for the helpful issue.
/cc @bemasc @randall77 @dvyukov

@bcmills
Copy link
Contributor

bcmills commented Jan 23, 2020

@toothrot, if I understand the report correctly, this is a regression introduced in 1.14. It should probably be a 1.14 release-blocker.

@bcmills
Copy link
Contributor

bcmills commented Jan 23, 2020

(Specifically, we should avoid releasing 1.14 with an unreliable race detector: the race detector is a really valuable tool for preventing DoS's and other security vulnerabilities, and we don't want to discourage its use by shipping a release that reports false-positives.)

@bemasc
Copy link
Contributor

bemasc commented Jan 23, 2020

:-(

e1446d9 is purely a performance optimization so it is safe to revert.

@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Go1.14 Jan 23, 2020
@gopherbot
Copy link

Change https://golang.org/cl/216158 mentions this issue: Revert "runtime: speed up receive on empty closed channel"

@gopherbot
Copy link

Change https://golang.org/cl/216099 mentions this issue: runtime/race: add test case for #36714

@josharian
Copy link
Contributor

@bemasc please do try the CL again in 1.15

changkun added a commit to golang-design/under-the-hood that referenced this issue Jan 25, 2020
@changkun
book: revert channel optimization
7758ce6 
See golang/go/issues/36714
@gopherbot
Copy link

Change https://golang.org/cl/216818 mentions this issue: runtime: speed up receive on empty closed channel

gopherbot pushed a commit that referenced this issue Mar 20, 2020
@ianlancetaylor
runtime/race: test that close synchronizes with read
635a830 
Add a test to ensure that the race detector sees that closing a
channel synchronizes with a read from that channel.
This test case failed when CL 181543 was in the tree.
CL 181543 was reverted in CL 216158; this adds a test to make
sure that we don't re-introduce the problem at a later date.

For #32529
For #36714

Change-Id: I5a40f744c67c3f8191d6ad822710c180880a7375
Reviewed-on: https://go-review.googlesource.com/c/go/+/216099
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Mar 22, 2020
@ianlancetaylor
runtime: speed up receive on empty closed channel
02e492f 
Currently, nonblocking receive on an open channel is about
700 times faster than nonblocking receive on a closed channel.
This change makes closed channels equally fast.

Fixes #32529.  Includes a correction based on #36714.

relevant benchstat output:
name                       old time/op    new time/op    delta
MakeChan/Byte-40            140ns ± 4%     137ns ± 7%   -2.38%  (p=0.023 n=17+19)
MakeChan/Int-40             174ns ± 5%     173ns ± 6%     ~     (p=0.437 n=18+19)
MakeChan/Ptr-40             315ns ±15%     301ns ±15%     ~     (p=0.051 n=20+20)
MakeChan/Struct/0-40        123ns ± 8%      99ns ±11%  -19.18%  (p=0.000 n=20+17)
MakeChan/Struct/32-40       297ns ± 8%     241ns ±18%  -19.13%  (p=0.000 n=20+20)
MakeChan/Struct/40-40       344ns ± 5%     273ns ±23%  -20.49%  (p=0.000 n=20+20)
ChanNonblocking-40         0.32ns ± 2%    0.32ns ± 2%   -1.25%  (p=0.000 n=19+18)
SelectUncontended-40       5.72ns ± 1%    5.71ns ± 2%     ~     (p=0.326 n=19+19)
SelectSyncContended-40     10.9µs ±10%    10.6µs ± 3%   -2.77%  (p=0.009 n=20+16)
SelectAsyncContended-40    1.00µs ± 0%    1.10µs ± 0%  +10.75%  (p=0.000 n=18+19)
SelectNonblock-40          1.22ns ± 2%    1.21ns ± 4%     ~     (p=0.141 n=18+19)
ChanUncontended-40          240ns ± 4%     233ns ± 4%   -2.82%  (p=0.000 n=20+20)
ChanContended-40           86.7µs ± 0%    82.7µs ± 0%   -4.64%  (p=0.000 n=20+19)
ChanSync-40                 294ns ± 7%     284ns ± 9%   -3.44%  (p=0.006 n=20+20)
ChanSyncWork-40            38.4µs ±19%    34.0µs ± 4%  -11.33%  (p=0.000 n=20+18)
ChanProdCons0-40           1.50µs ± 1%    1.63µs ± 0%   +8.53%  (p=0.000 n=19+19)
ChanProdCons10-40          1.17µs ± 0%    1.18µs ± 1%   +0.44%  (p=0.000 n=19+20)
ChanProdCons100-40          985ns ± 0%     959ns ± 1%   -2.64%  (p=0.000 n=20+20)
ChanProdConsWork0-40       1.50µs ± 0%    1.60µs ± 2%   +6.54%  (p=0.000 n=18+20)
ChanProdConsWork10-40      1.26µs ± 0%    1.26µs ± 2%   +0.40%  (p=0.015 n=20+19)
ChanProdConsWork100-40     1.27µs ± 0%    1.22µs ± 0%   -4.15%  (p=0.000 n=20+19)
SelectProdCons-40          1.50µs ± 1%    1.53µs ± 1%   +1.95%  (p=0.000 n=20+20)
ChanCreation-40            82.1ns ± 5%    81.6ns ± 7%     ~     (p=0.483 n=19+19)
ChanSem-40                  877ns ± 0%     719ns ± 0%  -17.98%  (p=0.000 n=18+19)
ChanPopular-40             1.75ms ± 2%    1.78ms ± 3%   +1.76%  (p=0.002 n=20+19)
ChanClosed-40               215ns ± 1%       0ns ± 6%  -99.82%  (p=0.000 n=20+18)

Previously committed in CL 181543 and reverted in CL 216158.

Change-Id: Ib767b08d724cfad03598d77271dbc1087485feb8
Reviewed-on: https://go-review.googlesource.com/c/go/+/216818
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
7 participants
@toothrot @josharian @zeebo @bemasc @ianlancetaylor @bcmills @gopherbot