The fix looks reasonable to me.

Hmm.. Though on further though I think that might open up a race condition. Consider:

1. numOpen = 1, and conn() decides to open a connection
2. The connection attempt fails and maybeOpenNewConnections() writes into db.openerCh
3. Another goroutine calls conn() again, sees numOpen = 1 and decides to open a connection
4. connectionOpener receives on db.openerCh and opens a third connection

Now you have three connections open, even though maxOpenConns is two.

I'm not sure whether a similar race condition already exists in the code or what the best way to fix it is. The most simple fix appears to be to change this code in conn:

    if db.maxOpen > 0 && db.numOpen >= db.maxOpen

to also take db.pendingOpens into account. But that might break some code path which already works today.

I have encountered this problem. Can we add it to the Go1.6 milestone? I can help work on the fix if that adds weight to my request.

I have a potential fix for this sitting in my local checkout. It's a little ugly, but I'd be happy to mail a CL if someone wants to review and propose edits

Of course - the changes proposed above cover the case of db.maxOpen+1 connection attempts being made while we are unable to connect to the database, but in order to cover db.maxOpen+2 or more, I found that there's an additional check required in the connectionOpener routine that processes queued connections. I came up with something like this:

 func (db *DB) connectionOpener() {
        for range db.openerCh {
                db.openNewConnection()
+
+               db.mu.Lock()
+               if db.numOpen == 0 {
+                       // If there are no open connections,
+                       // check to make sure we don't have any connection requests
+                       // waiting for this one to be freed.
+                       // This is done in a separate goroutine so that there is no chance
+                       // for us to cause a deadlock.
+                       go func() {
+                               db.mu.Lock()
+                               db.maybeOpenNewConnections()
+                               db.mu.Unlock()
+                       }()
+               }
+               db.mu.Unlock()
        }
 }

As I said this is kind of ugly, but as far as I can see we need to have some sort of check here if we want to preserve the ability to open connections in parallel.

I have written a test that reproduces this issue. The issue still exists in 1.5.1.

The proposed fix in DB.conn shown below allows the test to pass when db.maxOpen+2 when added to the Go 1.5.1 tree.

    db.numOpen++ // optimistically
    db.mu.Unlock()
    ci, err := db.driver.Open(db.dsn)
    if err != nil {
        db.mu.Lock()
        db.numOpen-- // correct for earlier optimism
        db.maybeOpenNewConnections()        // <--- fix
        db.mu.Unlock()
        return nil, err
    }

@johto Although the scenario you describe does allow a third connection to be opened, the change made in cce127a causes the extra connection to be closed before it is ever added to the pool or services any queries. Your suggestion to change

if db.maxOpen > 0 && db.numOpen >= db.maxOpen

to

if db.maxOpen > 0 && (db.numOpen+db.pendingOpens) >= db.maxOpen

seems correct to me. All database/sql tests still pass when I make that change.

@potocnyj I don't understand how the proposed fix doesn't handle db.maxOpen+2. db.maybeOpenNewConnections can queue up multiple requests per call if needed. Could you walk us through the sequence that doesn't work?

I will submit a CL with the new test and the above changes if there are no objections.

@ChrisHines I appear to have misspoken - without the additional change I proposed db.maxOpen+2 works in my test program, but db.maxOpen+3 hangs on the last connection. Adding my change (in addition to what was already proposed) works though, no matter how many connections I open simultaneously.

For reference, the full code I am using to test the failure is here: https://gist.github.com/potocnyj/fe05406010ad73660889

@potocnyj OK, thanks for the correction. Yes, I understand now. Opening db.maxOpen*2+n (for n>0) will not work. Once db.openNewConnection becomes the sole source of new connections—and they fail—we aren't triggering additional attempts to service the still pending requests.

I have additions to the proposed fix that work in this scenario, and it actually simplifies the logic.

I propose we consolidate the numOpen and pendingOpens counters into numOpen and always increment numOpen optimistically. Whenever an attempted connection fails we decrement numOpen and call db.maybeOpenNewConnections. This change simplifies type DB by removing a field. It also simplifies and harmonizes the logic in db.conn, db.openNewConnection and db.maybeOpenNewConnections.

During testing I also uncovered a panic that occurred when DB.Close was called while len(db.connRequests) > 0. DB.Close ranges over db.connRequests and closes each response channel. This resulted in the goroutines waiting on those channels to unblock and operate on a zero value sql.connRequest, which contained both a nil *driverConn and error. When that was returned to db.exec it caused a nil pointer panic. The fix is to use the comma-ok idiom to detect channel closure and return errDBClosed.

With these changes all database/sql tests pass, including the condition @potocnyj highlighted. I think this is good enough to mail a CL, and the changes are difficult to fully appreciate when described here.

CL https://golang.org/cl/14547 mentions this issue.