database/sql: connection leak with SetMaxOpenConns

[bradfitz]

Go 1.2 adds a new *DB.SetMaxOpenConns method.

But it contains a race.

From the email thread:

[golang-dev] code review 14611045: database/sql: Fix connection leak and potential
deadlock (issue #14611045)

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

[bradfitz]

Comment 1:

Fix in progress at https://golang.org/cl/14611045/

[bradfitz]

Comment 2:

This issue was updated by revision 478f4b6.

R=golang-dev, bradfitz, dave
CC=golang-dev
https://golang.org/cl/14642044
Committer: Brad Fitzpatrick 

[bradfitz]

Comment 3:

This issue was closed by revision 37db880.

Status changed to Fixed.