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.

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

data race in pool.go #488

Open
kmanley opened this issue Sep 16, 2020 · 3 comments
Open

data race in pool.go #488

kmanley opened this issue Sep 16, 2020 · 3 comments

Comments

@kmanley
Copy link

kmanley commented Sep 16, 2020

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/[email protected]/pool.go:127 +0x7d1
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/pool.go:183 +0x50
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Node).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/node.go:96 +0x151
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Cluster).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/cluster.go:92 +0x199
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Session).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/session.go:305 +0x1ba
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.Run()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/query.go:356 +0x208
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.RunWrite()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/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/[email protected]/pool.go:122 +0x163
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Pool).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/pool.go:183 +0x50
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Node).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/node.go:96 +0x151
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Cluster).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/cluster.go:92 +0x199
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.(*Session).Query()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/session.go:305 +0x1ba
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.Run()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/query.go:356 +0x208
  gopkg.in/rethinkdb/rethinkdb-go%2ev6.Term.RunWrite()
      /home/kevin/code/go/pkg/mod/gopkg.in/rethinkdb/[email protected]/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
image

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.

kmanley pushed a commit to kmanley/rethinkdb-go that referenced this issue Sep 16, 2020
@CMogilko
Copy link
Member

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
Copy link
Author

kmanley commented Sep 17, 2020

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants