Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1603)

Side by Side Diff: src/pkg/database/sql/sql.go

Issue 14611045: code review 14611045: database/sql: Fix connection leak and potential deadlock (Closed)
Patch Set: diff -r 062f1f0ea8ba https://code.google.com/p/go Created 10 years, 5 months ago
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments. Please Sign in to add in-line comments.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright 2011 The Go Authors. All rights reserved. 1 // Copyright 2011 The Go Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style 2 // Use of this source code is governed by a BSD-style
3 // license that can be found in the LICENSE file. 3 // license that can be found in the LICENSE file.
4 4
5 // Package sql provides a generic interface around SQL (or SQL-like) 5 // Package sql provides a generic interface around SQL (or SQL-like)
6 // databases. 6 // databases.
7 // 7 //
8 // The sql package must be used in conjunction with a database driver. 8 // The sql package must be used in conjunction with a database driver.
9 // See http://golang.org/s/sqldrivers for a list of drivers. 9 // See http://golang.org/s/sqldrivers for a list of drivers.
10 package sql 10 package sql
(...skipping 180 matching lines...) Expand 10 before | Expand all | Expand 10 after
191 // can be controlled with SetMaxIdleConns. 191 // can be controlled with SetMaxIdleConns.
192 type DB struct { 192 type DB struct {
193 driver driver.Driver 193 driver driver.Driver
194 dsn string 194 dsn string
195 195
196 mu sync.Mutex // protects following fields 196 mu sync.Mutex // protects following fields
197 freeConn *list.List // of *driverConn 197 freeConn *list.List // of *driverConn
198 connRequests *list.List // of connRequest 198 connRequests *list.List // of connRequest
199 numOpen int 199 numOpen int
200 pendingOpens int 200 pendingOpens int
201 // Used to sygnal the need for new connections 201 // Used to sygnal the need for new connections
bradfitz 2013/10/15 20:37:55 typo: signal
202 // a goroutine running connectionOpener() reads on this chan and 202 // a goroutine running connectionOpener() reads on this chan and
203 // maybeOpenNewConnections sends on the chan (one send per needed connec tion) 203 // maybeOpenNewConnections sends on the chan (one send per needed connec tion)
204 // It is closed during db.Close(). The close tells the connectionOpener 204 // It is closed during db.Close(). The close tells the connectionOpener
205 // goroutine to exit. 205 // goroutine to exit.
206 openerCh chan struct{} 206 openerCh chan struct{}
207 closed bool 207 closed bool
208 dep map[finalCloser]depSet 208 dep map[finalCloser]depSet
209 lastPut map[*driverConn]string // stacktrace of last conn's put; debug only 209 lastPut map[*driverConn]string // stacktrace of last conn's put; debug only
210 maxIdle int // zero means defaultMaxIdleConns; negat ive means 0 210 maxIdle int // zero means defaultMaxIdleConns; negat ive means 0
211 maxOpen int // <= 0 means unlimited 211 maxOpen int // <= 0 means unlimited
(...skipping 208 matching lines...) Expand 10 before | Expand all | Expand 10 after
420 func Open(driverName, dataSourceName string) (*DB, error) { 420 func Open(driverName, dataSourceName string) (*DB, error) {
421 driveri, ok := drivers[driverName] 421 driveri, ok := drivers[driverName]
422 if !ok { 422 if !ok {
423 return nil, fmt.Errorf("sql: unknown driver %q (forgotten import ?)", driverName) 423 return nil, fmt.Errorf("sql: unknown driver %q (forgotten import ?)", driverName)
424 } 424 }
425 db := &DB{ 425 db := &DB{
426 driver: driveri, 426 driver: driveri,
427 dsn: dataSourceName, 427 dsn: dataSourceName,
428 openerCh: make(chan struct{}, connectionRequestQueueSize), 428 openerCh: make(chan struct{}, connectionRequestQueueSize),
429 lastPut: make(map[*driverConn]string), 429 lastPut: make(map[*driverConn]string),
430 maxIdle: defaultMaxIdleConns,
430 } 431 }
431 db.freeConn = list.New() 432 db.freeConn = list.New()
432 db.connRequests = list.New() 433 db.connRequests = list.New()
433 go db.connectionOpener() 434 go db.connectionOpener()
434 return db, nil 435 return db, nil
435 } 436 }
436 437
437 // Ping verifies a connection to the database is still alive, 438 // Ping verifies a connection to the database is still alive,
438 // establishing a connection if necessary. 439 // establishing a connection if necessary.
439 func (db *DB) Ping() error { 440 func (db *DB) Ping() error {
(...skipping 35 matching lines...) Expand 10 before | Expand all | Expand 10 after
475 err1 := fn() 476 err1 := fn()
476 if err1 != nil { 477 if err1 != nil {
477 err = err1 478 err = err1
478 } 479 }
479 } 480 }
480 return err 481 return err
481 } 482 }
482 483
483 const defaultMaxIdleConns = 2 484 const defaultMaxIdleConns = 2
484 485
485 func (db *DB) maxIdleConnsLocked() int {
bradfitz 2013/10/15 20:37:55 why is this being removed? can't you fix this mor
486 n := db.maxIdle
487 switch {
488 case n == 0:
489 // TODO(bradfitz): ask driver, if supported, for its default pre ference
490 return defaultMaxIdleConns
491 case n < 0:
492 return 0
493 default:
494 return n
495 }
496 }
497
498 // SetMaxIdleConns sets the maximum number of connections in the idle 486 // SetMaxIdleConns sets the maximum number of connections in the idle
499 // connection pool. 487 // connection pool.
500 // 488 //
501 // If MaxOpenConns is greater than 0 but less than the new MaxIdleConns 489 // If MaxOpenConns is greater than 0 but less than the new MaxIdleConns
502 // then the new MaxIdleConns will be reduced to match the MaxOpenConns limit 490 // then the new MaxIdleConns will be reduced to match the MaxOpenConns limit
503 // 491 //
504 // If n <= 0, no idle connections are retained. 492 // If n <= 0, no idle connections are retained.
505 func (db *DB) SetMaxIdleConns(n int) { 493 func (db *DB) SetMaxIdleConns(n int) {
506 db.mu.Lock() 494 db.mu.Lock()
507 defer db.mu.Unlock() 495 defer db.mu.Unlock()
508 » if n > 0 { 496 » switch {
509 » » db.maxIdle = n 497 » case n == 0:
510 » } else { 498 » » // TODO(bradfitz): ask driver, if supported, for its default pre ference
499 » » db.maxIdle = defaultMaxIdleConns
500 » case n < 0:
511 // No idle connections. 501 // No idle connections.
512 db.maxIdle = -1 502 db.maxIdle = -1
503 default:
504 db.maxIdle = n
513 } 505 }
514 // Make sure maxIdle doesn't exceed maxOpen 506 // Make sure maxIdle doesn't exceed maxOpen
515 » if db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen { 507 » if db.maxOpen > 0 && db.maxIdle > db.maxOpen {
516 db.maxIdle = db.maxOpen 508 db.maxIdle = db.maxOpen
517 } 509 }
518 » for db.freeConn.Len() > db.maxIdleConnsLocked() { 510 » for db.freeConn.Len() > db.maxIdle {
519 dc := db.freeConn.Back().Value.(*driverConn) 511 dc := db.freeConn.Back().Value.(*driverConn)
520 dc.listElem = nil 512 dc.listElem = nil
521 db.freeConn.Remove(db.freeConn.Back()) 513 db.freeConn.Remove(db.freeConn.Back())
522 go dc.Close() 514 go dc.Close()
523 } 515 }
524 } 516 }
525 517
526 // SetMaxOpenConns sets the maximum number of open connections to the database. 518 // SetMaxOpenConns sets the maximum number of open connections to the database.
527 // 519 //
528 // If MaxIdleConns is greater than 0 and the new MaxOpenConns is less than 520 // If MaxIdleConns is greater than 0 and the new MaxOpenConns is less than
529 // MaxIdleConns, then MaxIdleConns will be reduced to match the new 521 // MaxIdleConns, then MaxIdleConns will be reduced to match the new
530 // MaxOpenConns limit 522 // MaxOpenConns limit
531 // 523 //
532 // If n <= 0, then there is no limit on the number of open connections. 524 // If n <= 0, then there is no limit on the number of open connections.
533 // The default is 0 (unlimited). 525 // The default is 0 (unlimited).
534 func (db *DB) SetMaxOpenConns(n int) { 526 func (db *DB) SetMaxOpenConns(n int) {
535 db.mu.Lock() 527 db.mu.Lock()
536 db.maxOpen = n 528 db.maxOpen = n
537 if n < 0 { 529 if n < 0 {
538 db.maxOpen = 0 530 db.maxOpen = 0
539 } 531 }
540 » syncMaxIdle := db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen 532 » syncMaxIdle := db.maxOpen > 0 && db.maxIdle > db.maxOpen
541 db.mu.Unlock() 533 db.mu.Unlock()
542 if syncMaxIdle { 534 if syncMaxIdle {
543 db.SetMaxIdleConns(n) 535 db.SetMaxIdleConns(n)
544 } 536 }
545 } 537 }
546 538
547 // Assumes db.mu is locked. 539 // Assumes db.mu is locked.
548 // If there are connRequests and the connection limit hasn't been reached, 540 // If there are connRequests and the connection limit hasn't been reached,
549 // then tell the connectionOpener to open new connections. 541 // then tell the connectionOpener to open new connections.
550 func (db *DB) maybeOpenNewConnections() { 542 func (db *DB) maybeOpenNewConnections() {
(...skipping 24 matching lines...) Expand all
575 db.mu.Lock() 567 db.mu.Lock()
576 defer db.mu.Unlock() 568 defer db.mu.Unlock()
577 if db.closed { 569 if db.closed {
578 if err == nil { 570 if err == nil {
579 ci.Close() 571 ci.Close()
580 } 572 }
581 return 573 return
582 } 574 }
583 db.pendingOpens-- 575 db.pendingOpens--
584 if err != nil { 576 if err != nil {
585 » » db.putConnDBLocked(nil, err) 577 » » db.putConnDBLocked(nil, err, false)
586 return 578 return
587 } 579 }
588 dc := &driverConn{ 580 dc := &driverConn{
589 db: db, 581 db: db,
590 ci: ci, 582 ci: ci,
591 } 583 }
592 db.addDepLocked(dc, dc) 584 db.addDepLocked(dc, dc)
593 db.numOpen++ 585 db.numOpen++
594 » db.putConnDBLocked(dc, err) 586 » // hierro: Force the connection to be added to the idle list,
bradfitz 2013/10/15 20:37:55 remove "hierro: ".
587 » // even if it momentarily violates the maximum number
588 » // of idle connections. Otherwise, the connection would
589 » // be leaked when this sequence of events happens.
590 » // - A new connection is requested while there are no idle
591 » // connections. A signal is sent via openerCh to open a new one.
592 » // - The gorutine running connectionOpener() gets the signal and
bradfitz 2013/10/15 20:37:55 typo: goroutine
593 » // starts opening a new connection. Since that involves sockets,
bradfitz 2013/10/15 20:37:55 Change the "Since that" sentence to just "It block
594 » // it gets suspended.
595 » // - In the meantime, maxIdle (2 by default) + 1 connections which
596 » // were already open but busy are released. This fullfills the
597 » // connection requested by the first goroutine and makes the idle
598 » // list full.
599 » // - The new connection finishes opening, execution resumes at
600 » // openNewConnection() and putConnDBLocked() gets called, but there
601 » // are not pending requests and the idle list is already full.
602 » // - The connection ends up discarded without being closed.
603 » // - Eventually, the number of leaked connections reaches
604 » // MIN(maxOpenConnections, max_connections_the_server_allows) and
605 » // either deadlocks or stops working, also bringing the whole database
606 » // server down.
607 » //
608 » // There are two possible fixes for this problem: either close and
609 » // discard the connection or add it to the idle list. I've opted for
610 » // the latter because by the time we realize the connection is not
611 » // needed anymore it's already open and ready to use. It would be
612 » // a waste to close it when new connections will probably be opened
613 » // in the near future, even when this momentarily violates the
614 » // maximum number of idle connections.
615 » db.putConnDBLocked(dc, nil, true)
595 } 616 }
596 617
597 // connRequest represents one request for a new connection 618 // connRequest represents one request for a new connection
598 // When there are no idle connections available, DB.conn will create 619 // When there are no idle connections available, DB.conn will create
599 // a new connRequest and put it on the db.connRequests list. 620 // a new connRequest and put it on the db.connRequests list.
600 type connRequest chan<- interface{} // takes either a *driverConn or an error 621 type connRequest chan<- interface{} // takes either a *driverConn or an error
601 622
602 var errDBClosed = errors.New("sql: database is closed") 623 var errDBClosed = errors.New("sql: database is closed")
603 624
604 // conn returns a newly-opened or cached *driverConn 625 // conn returns a newly-opened or cached *driverConn
(...skipping 141 matching lines...) Expand 10 before | Expand all | Expand 10 after
746 // as closed. Decrement the open count. 767 // as closed. Decrement the open count.
747 db.numOpen-- 768 db.numOpen--
748 db.maybeOpenNewConnections() 769 db.maybeOpenNewConnections()
749 db.mu.Unlock() 770 db.mu.Unlock()
750 dc.Close() 771 dc.Close()
751 return 772 return
752 } 773 }
753 if putConnHook != nil { 774 if putConnHook != nil {
754 putConnHook(db, dc) 775 putConnHook(db, dc)
755 } 776 }
756 » added := db.putConnDBLocked(dc, nil) 777 » added := db.putConnDBLocked(dc, nil, false)
757 db.mu.Unlock() 778 db.mu.Unlock()
758 779
759 if !added { 780 if !added {
760 dc.Close() 781 dc.Close()
761 } 782 }
762 } 783 }
763 784
764 // Satisfy a connRequest or put the driverConn in the idle pool and return true 785 // Satisfy a connRequest or put the driverConn in the idle pool and return true
765 // or return false. 786 // or return false.
766 // putConnDBLocked will satisfy a connRequest if there is one, or it will 787 // putConnDBLocked will satisfy a connRequest if there is one, or it will
767 // return the *driverConn to the freeConn list if err != nil and the idle 788 // return the *driverConn to the freeConn list if err != nil and the idle
768 // connection limit would not be reached. 789 // connection limit would not be reached or if force is true.
769 // If err != nil, the value of dc is ignored. 790 // If err != nil, the value of dc is ignored.
770 // If err == nil, then dc must not equal nil. 791 // If err == nil, then dc must not equal nil.
771 // If a connRequest was fullfilled or the *driverConn was placed in the 792 // If a connRequest was fullfilled or the *driverConn was placed in the
772 // freeConn list, then true is returned, otherwise false is returned. 793 // freeConn list, then true is returned, otherwise false is returned.
773 func (db *DB) putConnDBLocked(dc *driverConn, err error) bool { 794 func (db *DB) putConnDBLocked(dc *driverConn, err error, force bool) bool {
774 if db.connRequests.Len() > 0 { 795 if db.connRequests.Len() > 0 {
775 req := db.connRequests.Front().Value.(connRequest) 796 req := db.connRequests.Front().Value.(connRequest)
776 db.connRequests.Remove(db.connRequests.Front()) 797 db.connRequests.Remove(db.connRequests.Front())
777 if err != nil { 798 if err != nil {
778 req <- err 799 req <- err
779 } else { 800 } else {
780 dc.inUse = true 801 dc.inUse = true
781 req <- dc 802 req <- dc
782 } 803 }
783 return true 804 return true
784 » } else if err == nil && !db.closed && db.maxIdleConnsLocked() > 0 && db. maxIdleConnsLocked() > db.freeConn.Len() { 805 » } else if err == nil && !db.closed && (force || db.maxIdle > db.freeConn .Len()) {
785 dc.listElem = db.freeConn.PushFront(dc) 806 dc.listElem = db.freeConn.PushFront(dc)
786 return true 807 return true
787 } 808 }
788 return false 809 return false
789 } 810 }
790 811
791 // Prepare creates a prepared statement for later queries or executions. 812 // Prepare creates a prepared statement for later queries or executions.
792 // Multiple queries or executions may be run concurrently from the 813 // Multiple queries or executions may be run concurrently from the
793 // returned statement. 814 // returned statement.
794 func (db *DB) Prepare(query string) (*Stmt, error) { 815 func (db *DB) Prepare(query string) (*Stmt, error) {
(...skipping 857 matching lines...) Expand 10 before | Expand all | Expand 10 after
1652 var buf [2 << 10]byte 1673 var buf [2 << 10]byte
1653 return string(buf[:runtime.Stack(buf[:], false)]) 1674 return string(buf[:runtime.Stack(buf[:], false)])
1654 } 1675 }
1655 1676
1656 // withLock runs while holding lk. 1677 // withLock runs while holding lk.
1657 func withLock(lk sync.Locker, fn func()) { 1678 func withLock(lk sync.Locker, fn func()) {
1658 lk.Lock() 1679 lk.Lock()
1659 fn() 1680 fn()
1660 lk.Unlock() 1681 lk.Unlock()
1661 } 1682 }
OLDNEW
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b