FAQ
Reviewers: golang-dev1,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go


Description:
database/sql: Fix connection leak and potential deadlock

CL 10726044 introduced a race condition which causes connections
to be leaked under certain circumstances. If SetMaxOpenConns is
used, the application eventually deadlocks. Otherwise, the number
of open connections just keep growing indefinitely.

Please review this at https://codereview.appspot.com/14611045/

Affected files (+16, -24 lines):
    M src/pkg/database/sql/sql.go


Index: src/pkg/database/sql/sql.go
===================================================================
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -427,6 +427,7 @@
     dsn: dataSourceName,
     openerCh: make(chan struct{}, connectionRequestQueueSize),
     lastPut: make(map[*driverConn]string),
+ maxIdle: defaultMaxIdleConns,
    }
    db.freeConn = list.New()
    db.connRequests = list.New()
@@ -482,19 +483,6 @@

   const defaultMaxIdleConns = 2

-func (db *DB) maxIdleConnsLocked() int {
- n := db.maxIdle
- switch {
- case n == 0:
- // TODO(bradfitz): ask driver, if supported, for its default preference
- return defaultMaxIdleConns
- case n < 0:
- return 0
- default:
- return n
- }
-}
-
   // SetMaxIdleConns sets the maximum number of connections in the idle
   // connection pool.
   //
@@ -505,17 +493,21 @@
   func (db *DB) SetMaxIdleConns(n int) {
    db.mu.Lock()
    defer db.mu.Unlock()
- if n > 0 {
- db.maxIdle = n
- } else {
+ switch {
+ case n == 0:
+ // TODO(bradfitz): ask driver, if supported, for its default preference
+ db.maxIdle = defaultMaxIdleConns
+ case n < 0:
     // No idle connections.
     db.maxIdle = -1
+ default:
+ db.maxIdle = n
    }
    // Make sure maxIdle doesn't exceed maxOpen
- if db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen {
+ if db.maxOpen > 0 && db.maxIdle > db.maxOpen {
     db.maxIdle = db.maxOpen
    }
- for db.freeConn.Len() > db.maxIdleConnsLocked() {
+ for db.freeConn.Len() > db.maxIdle {
     dc := db.freeConn.Back().Value.(*driverConn)
     dc.listElem = nil
     db.freeConn.Remove(db.freeConn.Back())
@@ -537,7 +529,7 @@
    if n < 0 {
     db.maxOpen = 0
    }
- syncMaxIdle := db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen
+ syncMaxIdle := db.maxOpen > 0 && db.maxIdle > db.maxOpen
    db.mu.Unlock()
    if syncMaxIdle {
     db.SetMaxIdleConns(n)
@@ -582,7 +574,7 @@
    }
    db.pendingOpens--
    if err != nil {
- db.putConnDBLocked(nil, err)
+ db.putConnDBLocked(nil, err, false)
     return
    }
    dc := &driverConn{
@@ -591,7 +583,7 @@
    }
    db.addDepLocked(dc, dc)
    db.numOpen++
- db.putConnDBLocked(dc, err)
+ db.putConnDBLocked(dc, nil, true)
   }

   // connRequest represents one request for a new connection
@@ -753,7 +745,7 @@
    if putConnHook != nil {
     putConnHook(db, dc)
    }
- added := db.putConnDBLocked(dc, nil)
+ added := db.putConnDBLocked(dc, nil, false)
    db.mu.Unlock()

    if !added {
@@ -770,7 +762,7 @@
   // If err == nil, then dc must not equal nil.
   // If a connRequest was fullfilled or the *driverConn was placed in the
   // freeConn list, then true is returned, otherwise false is returned.
-func (db *DB) putConnDBLocked(dc *driverConn, err error) bool {
+func (db *DB) putConnDBLocked(dc *driverConn, err error, force bool) bool {
    if db.connRequests.Len() > 0 {
     req := db.connRequests.Front().Value.(connRequest)
     db.connRequests.Remove(db.connRequests.Front())
@@ -781,7 +773,7 @@
      req <- dc
     }
     return true
- } else if err == nil && !db.closed && db.maxIdleConnsLocked() > 0 &&
db.maxIdleConnsLocked() > db.freeConn.Len() {
+ } else if err == nil && !db.closed && (force || db.maxIdle >
db.freeConn.Len()) {
     dc.listElem = db.freeConn.PushFront(dc)
     return true
    }


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

Discussion Posts

Follow ups

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 1 of 5 | next ›
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 11, '13 at 7:04p
activeOct 11, '13 at 8:15p
posts5
users2
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase