Data race in close and panic on closed channel

[davecheney]

This test exposes and data race and crashes.

func TestDoubleClose(t *testing.T) {
        c := New()
        start := make(chan bool)
        const n = 10
        var wg sync.WaitGroup
        wg.Add(n)
        for i := 0; i < n; i++ {
                go func() {
                        defer wg.Done()
                        <-start
                        c.Close()
                }()
        }
        close(start)
        wg.Wait()
}

lucky(~/src/github.com/goburrow/cache) % go test -race
==================
WARNING: DATA RACE
Write at 0x00c420136720 by goroutine 18:
  runtime.closechan()
      /home/dfc/go/src/runtime/chan.go:320 +0x0
  github.com/goburrow/cache.(*localCache).processEntries()
      /home/dfc/src/github.com/goburrow/cache/local.go:172 +0x305

Previous read at 0x00c420136720 by goroutine 23:
  runtime.chansend()
      /home/dfc/go/src/runtime/chan.go:128 +0x0
  github.com/goburrow/cache.(*localCache).Close()
      /home/dfc/src/github.com/goburrow/cache/local.go:80 +0xb9
  github.com/goburrow/cache.TestDoubleClose.func1()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:144 +0x8b

Goroutine 18 (running) created at:
  github.com/goburrow/cache.(*localCache).init()
      /home/dfc/src/github.com/goburrow/cache/local.go:74 +0x2be
  github.com/goburrow/cache.New()
      /home/dfc/src/github.com/goburrow/cache/local.go:328 +0x1ed
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:135 +0x4d
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228

Goroutine 23 (running) created at:
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:145 +0x126
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228
==================
==================
WARNING: DATA RACE
Write at 0x00c42013c150 by goroutine 20:
  github.com/goburrow/cache.(*localCache).Close()
      /home/dfc/src/github.com/goburrow/cache/local.go:84 +0x105
  github.com/goburrow/cache.TestDoubleClose.func1()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:144 +0x8b

Previous read at 0x00c42013c150 by goroutine 28:
  github.com/goburrow/cache.(*localCache).Close()
      /home/dfc/src/github.com/goburrow/cache/local.go:83 +0xca
  github.com/goburrow/cache.TestDoubleClose.func1()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:144 +0x8b

Goroutine 20 (running) created at:
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:145 +0x126
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228

Goroutine 28 (running) created at:
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:145 +0x126
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228
==================
==================
WARNING: DATA RACE
Read at 0x00c420136720 by goroutine 25:
  runtime.chansend()
      /home/dfc/go/src/runtime/chan.go:128 +0x0
  github.com/goburrow/cache.(*localCache).Close()
      /home/dfc/src/github.com/goburrow/cache/local.go:80 +0xb9
  github.com/goburrow/cache.TestDoubleClose.func1()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:144 +0x8b

Previous write at 0x00c420136720 by goroutine 18:
  runtime.closechan()
      /home/dfc/go/src/runtime/chan.go:320 +0x0
  github.com/goburrow/cache.(*localCache).processEntries()
      /home/dfc/src/github.com/goburrow/cache/local.go:172 +0x305

Goroutine 25 (running) created at:
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:145 +0x126
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228

Goroutine 18 (running) created at:
  github.com/goburrow/cache.(*localCache).init()
      /home/dfc/src/github.com/goburrow/cache/local.go:74 +0x2be
  github.com/goburrow/cache.New()
      /home/dfc/src/github.com/goburrow/cache/local.go:328 +0x1ed
  github.com/goburrow/cache.TestDoubleClose()
      /home/dfc/src/github.com/goburrow/cache/local_test.go:135 +0x4d
  testing.tRunner()
      /home/dfc/go/src/testing/testing.go:679 +0x228
==================
panic: send on closed channel

goroutine 60 [running]:
github.com/goburrow/cache.(*localCache).Close(0xc42013c0b0, 0xc420136780, 0x0)
        /home/dfc/src/github.com/goburrow/cache/local.go:80 +0xba
github.com/goburrow/cache.TestDoubleClose.func1(0xc42012c130, 0xc420136780, 0x6e25c0, 0xc42013c0b0)
        /home/dfc/src/github.com/goburrow/cache/local_test.go:144 +0x8c
created by github.com/goburrow/cache.TestDoubleClose
        /home/dfc/src/github.com/goburrow/cache/local_test.go:145 +0x127

[nqv]

My bad I didn't write Close function with concurrency support in mind. May I ask you what is the proper way to deal with multiple closing Go channel? Should I just use sync.Mutex?

(There is also an issue with writing on Closed cache which should be fixed)

[davecheney]

There are two problems,

one is setting closeCh to nil, which exposes a data race, the other is closing a channel twice. The best ways seems to be a mutex.

On Tue, Nov 29, 2016 at 11:49 AM, Quoc-Viet Nguyen <notifications@github.com

wrote:

My bad I didn't write Close function with concurrency support in mind. May I ask you what is the proper way to deal with multiple closing Go channel? Should I just use sync.Mutex?

(There is also an issue with writing on Closed cache)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/goburrow/cache/issues/4#issuecomment-263442657, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcA1wFdZNVZ-x6HKj2vWaL3hvFyZ8dks5rC3aQgaJpZM4K-an4 .