Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Connection pool is not safe #497

Closed
CodyDWJones opened this issue Sep 10, 2021 · 3 comments
Closed

Connection pool is not safe #497

CodyDWJones opened this issue Sep 10, 2021 · 3 comments

Comments

@CodyDWJones
Copy link
Contributor

CodyDWJones commented Sep 10, 2021

Describe the bug
The connection pool is only useful when writing concurrent code, but it doesn't actually stop multiple goroutines from accessing the same connection simultaneously.

To Reproduce
The PR includes a test suite that reproduces the issue with the old Pool code, so long as it's edited to include the afterAcquire and beforeRelease test hooks.

Expected behavior

  • Pool.Exec(), Pool.Ping(), Pool.Query(), and Pool.Server() are safe for concurrent use. Two calls can only use the same connection serially.
  • When a query is submitted to the pool,
    • If there is an unused connection in the pool, it will be used to run the query immediately.
    • Otherwise, the pool will wait for a connection to become available and then run the query.
  • A connection is only returned to the pool when it is safe to do so.
    • Cursors perform their work via the connection, which means if the pool returns a cursor, the connection cannot be returned to the pool until the cursor is closed.
  • A mutex is not used in the hot path as per the expectation set in data race in pool.go #488.

Screenshots
n/a

System info
n/a

Additional context
Problems with the prior pool implementation:

  • Pool.conn() uses the connections in round-robin order without checking if they are still in use. It appears queries and cursors can be multiplexed on one connection, so this would work fine in the single goroutine case. However, if multiple goroutines call the Pool there is no guarantee each Connection will only be used by one goroutine at a time.

  • Similarly there is a race condition in the lines:

pos := atomic.AddInt32(&p.pointer, 1)
if pos == int32(len(p.conns)) {
	atomic.StoreInt32(&p.pointer, 0)
}
pos = pos % int32(len(p.conns))

Suppose pointer == 9 and len(conns) == 10. Three goroutines could run in this sequence:

GR 1: pointer=10                pointer=0                uses conns[0]
GR 2:              pointer=11                            uses conns[1]
GR 3:                                       pointer=1    uses conns[1]
      ---------time------->

Because there is no "in use" check, two of the goroutines end up using the same Connection.

@srh
Copy link

srh commented Sep 14, 2021

It sounds like a legitimate bug report. I don't have a timeline for a fix.

@CodyDWJones
Copy link
Contributor Author

I have just submitted a proposed fix in #498.

@CodyDWJones
Copy link
Contributor Author

Not an issue; the Connection and Cursor docs say they do not support concurrent calls, but those comments are very old and aren't actually true.

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

Successfully merging a pull request may close this issue.

2 participants