data race in pool.go

[kmanley]

Describe the bug
pool.go has a race condition around p.conns

WARNING: DATA RACE
Write at 0x00c00010e130 by goroutine 67:
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).conn()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/pool.go:127 +0x7d1
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/pool.go:183 +0x50
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Node).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/node.go:96 +0x151
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Cluster).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/cluster.go:92 +0x199
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Session).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/session.go:305 +0x1ba
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.Run()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/query.go:356 +0x208
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.RunWrite()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/query.go:369 +0x140

Previous read at 0x00c00010e130 by goroutine 122:
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).conn()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/pool.go:122 +0x163
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/pool.go:183 +0x50
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Node).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/node.go:96 +0x151
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Cluster).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/cluster.go:92 +0x199
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Session).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/session.go:305 +0x1ba
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.Run()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/query.go:356 +0x208
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.RunWrite()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/rethinkdb-go.v6@v6.2.1/query.go:369 +0x140

To Reproduce
Steps to reproduce the behavior:

• I found this while running proprietary code that is highly concurrent. I don't have time to create a simpler test case to repro it, but looking at the code the problem is obvious. The mutex needs to be locked before the first read of p.conns

Expected behavior

• No race

Screenshots
Applicable code

System info

• Ubuntu 18.04.4 under WSL2/Windows 10
• RethinkDB Version: 2.3.6.srh.1~0bionic

Additional context
Add any other context about the problem here.

[CMogilko]

Hello, thank you for your contribution.
But it's okay-race. Because p.conns[pos] is double checked inside the mutex. It is an optimization. conn() function is very frequently used and proposed in #489 solution to lock mutex at the beginning is very expensive solution to a problem solved by double checking.

[kmanley]

I'm not sure any race is okay. Please see:

https://groups.google.com/g/golang-nuts/c/EHHMCdcenc8
"This has come up quite a few times before on this list (in one guise or another), and the consensus seems to be no, there is no such thing as a benign data race in go."

and

https://groups.google.com/g/golang-nuts/c/MB1QmhDd_Rk
"Go does have undefined behaviour: if your program has a race condition, the behaviour is undefined. If you have a race condition you can see inconsistent and seemingly impossible states in your program."

[CMogilko]

It is common thing https://en.wikipedia.org/wiki/Double-checked_locking