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

  • Brad Fitzpatrick at Oct 11, 2013 at 7:07 pm
    No test to demonstrate the problem/fix?

    What are the certain conditions?
      On Oct 11, 2013 9:04 AM, wrote:

    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/<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<golang-dev%2Bunsubscribe@googlegroups.com>
    .
    For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/opt_out>
    .
    --

    ---
    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.
  • Alberto Garcia Hierro at Oct 11, 2013 at 7:27 pm

    On 2013/10/11 19:07:44, bradfitz wrote:
    No test to demonstrate the problem/fix?
    I could not reproduce the problem with a small program, but it
    manifested itself repeatedly in one of our high traffic sites every few
    days. The timing is pretty sensitive and the window for the leak to
    occur is very small. What basically happens is:

    - A new connection is requested while there are no idle connections. A
    signal is sent via openerCh to open a new one.
    - The gorutine running connectionOpener() gets the signal and starts
    opening a new connection. Since that involves sockets, it gets
    suspended.
    - In the meantime, maxIdle (2 by default) + 1 connections which were
    already open but busy are released. This fulfills the connection
    requested by the first goroutine and makes the idle list full.
    - The new connection finishes opening, execution resumes at
    openNewConnection() and it calls putConnDBLocked() without checking the
    return value, assuming the connection will either fulfill a pending
    request or be added to the idle queue, but that's not always the case.
    In this situation, the connection ends up discarded without being
    closed, but counted (numOpen is incremented).
    - Eventually, the number of leaked connections reaches
    MIN(maxOpenConnections, max_connections_the_server_allows) and either
    deadlocks or stops working bringing the whole database server down.

    This was a total pain to track down (in part because we have a ORM built
    on top of database/sql and I started digging there before looking in
    database/sql), but I think this explanation makes it pretty evident.

    As for the fix, I saw basically two options: either close and discard
    the connection or add it to the idle list. I opted for the latter
    because by the time we realize the connection is not needed anymore is
    already open and ready to use. It would be a waste to close it when the
    app will probably need to open a new one in the near future, even when
    this fix momentarily violates the maxIdle constraint.

    Regards,
    Alberto

    https://codereview.appspot.com/14611045/

    --

    ---
    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.
  • Brad Fitzpatrick at Oct 11, 2013 at 7:34 pm
    Thanks for the explanation. Makes sense. The http client code has similar
    logic for opening new TCP connections to hosts.

    Please document the new force parameter. And put a subset of this email
    into a comment near it maybe.
      On Oct 11, 2013 9:27 AM, wrote:
    On 2013/10/11 19:07:44, bradfitz wrote:

    No test to demonstrate the problem/fix?
    I could not reproduce the problem with a small program, but it
    manifested itself repeatedly in one of our high traffic sites every few
    days. The timing is pretty sensitive and the window for the leak to
    occur is very small. What basically happens is:

    - A new connection is requested while there are no idle connections. A
    signal is sent via openerCh to open a new one.
    - The gorutine running connectionOpener() gets the signal and starts
    opening a new connection. Since that involves sockets, it gets
    suspended.
    - In the meantime, maxIdle (2 by default) + 1 connections which were
    already open but busy are released. This fulfills the connection
    requested by the first goroutine and makes the idle list full.
    - The new connection finishes opening, execution resumes at
    openNewConnection() and it calls putConnDBLocked() without checking the
    return value, assuming the connection will either fulfill a pending
    request or be added to the idle queue, but that's not always the case.
    In this situation, the connection ends up discarded without being
    closed, but counted (numOpen is incremented).
    - Eventually, the number of leaked connections reaches
    MIN(maxOpenConnections, max_connections_the_server_**allows) and either
    deadlocks or stops working bringing the whole database server down.

    This was a total pain to track down (in part because we have a ORM built
    on top of database/sql and I started digging there before looking in
    database/sql), but I think this explanation makes it pretty evident.

    As for the fix, I saw basically two options: either close and discard
    the connection or add it to the idle list. I opted for the latter
    because by the time we realize the connection is not needed anymore is
    already open and ready to use. It would be a waste to close it when the
    app will probably need to open a new one in the near future, even when
    this fix momentarily violates the maxIdle constraint.

    Regards,
    Alberto

    https://codereview.appspot.**com/14611045/<https://codereview.appspot.com/14611045/>
    --

    ---
    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.
  • Alberto Garcia Hierro at Oct 11, 2013 at 8:15 pm

    On 2013/10/11 19:34:42, bradfitz wrote:
    Thanks for the explanation. Makes sense. The http client code has similar
    logic for opening new TCP connections to hosts.
    Please document the new force parameter. And put a subset of this email
    into a comment near it maybe.
    My pleasure. I'm glad we caught this before the 1.2. I've updated the
    patch according to your comments. PTAL.

    Regards,
    Alberto

    https://codereview.appspot.com/14611045/

    --

    ---
    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.

Related Discussions

Discussion Navigation
viewthread | post
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 © 2021 Grokbase