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

Issue 14611045: code review 14611045: database/sql: Fix connection leak and potential deadlock (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by Hierro
Modified:
8 years, 3 months ago
CC:
golang-dev, bradfitz, tad.glines_gmail.com, bketelsen
Visibility:
Public.

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. Fixes issue 6593

Patch Set 1 #

Patch Set 2 : diff -r 062f1f0ea8ba https://code.google.com/p/go #

Patch Set 3 : diff -r 062f1f0ea8ba https://code.google.com/p/go #

Patch Set 4 : diff -r 062f1f0ea8ba https://code.google.com/p/go #

Patch Set 5 : diff -r 062f1f0ea8ba https://code.google.com/p/go #

Total comments: 5

Patch Set 6 : diff -r 9d0d95344a6c https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -3 lines) Patch
M src/pkg/database/sql/fakedb_test.go View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M src/pkg/database/sql/sql.go View 1 2 3 4 5 1 chunk +6 lines, -3 lines 0 comments Download
M src/pkg/database/sql/sql_test.go View 1 2 3 4 5 1 chunk +51 lines, -0 lines 0 comments Download

Messages

Total messages: 28
Hierro
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
8 years, 3 months ago (2013-10-11 19:04:28 UTC) #1
bradfitz
No test to demonstrate the problem/fix? What are the certain conditions? On Oct 11, 2013 ...
8 years, 3 months ago (2013-10-11 19:07:44 UTC) #2
Hierro
On 2013/10/11 19:07:44, bradfitz wrote: > No test to demonstrate the problem/fix? I could not ...
8 years, 3 months ago (2013-10-11 19:27:17 UTC) #3
bradfitz
Thanks for the explanation. Makes sense. The http client code has similar logic for opening ...
8 years, 3 months ago (2013-10-11 19:34:42 UTC) #4
Hierro
On 2013/10/11 19:34:42, bradfitz wrote: > Thanks for the explanation. Makes sense. The http client ...
8 years, 3 months ago (2013-10-11 20:15:57 UTC) #5
Hierro
It seems that there are still some connection leaks even after applying this patch, although ...
8 years, 3 months ago (2013-10-12 09:55:58 UTC) #6
Hierro
On 2013/10/12 09:55:58, Hierro wrote: > It seems that there are still some connection leaks ...
8 years, 3 months ago (2013-10-12 10:57:42 UTC) #7
bradfitz
Please report back. Time is running short on Go 1.2, but this is why we ...
8 years, 3 months ago (2013-10-12 21:00:54 UTC) #8
Hierro
On 2013/10/12 21:00:54, bradfitz wrote: > Please report back. Time is running short on Go ...
8 years, 3 months ago (2013-10-12 22:13:07 UTC) #9
bradfitz
Don't use time.Sleep in the fake driver. Control operation order with channels. On Oct 12, ...
8 years, 3 months ago (2013-10-12 22:36:23 UTC) #10
Hierro
Good news everyone! The test has been running for 12 hours and db.numOpen matches the ...
8 years, 3 months ago (2013-10-13 11:00:31 UTC) #11
bradfitz
Please mail your CLs when they're ready. I'll look Tuesday morning US/Pacific. On Oct 13, ...
8 years, 3 months ago (2013-10-13 17:55:02 UTC) #12
bradfitz
https://codereview.appspot.com/14611045/diff/14001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (left): https://codereview.appspot.com/14611045/diff/14001/src/pkg/database/sql/sql.go#oldcode485 src/pkg/database/sql/sql.go:485: func (db *DB) maxIdleConnsLocked() int { why is this ...
8 years, 3 months ago (2013-10-15 20:37:55 UTC) #13
Hierro
Hi Brad, Thanks for the review, I'll keep your comments in mind when submitting the ...
8 years, 3 months ago (2013-10-15 22:04:53 UTC) #14
bradfitz
One CL. You don't have much time, though. Today or tomorrow morning would be best. ...
8 years, 3 months ago (2013-10-15 22:33:17 UTC) #15
Hierro
Hi Brad, I've just submitted the first CL. Please, take a look at it https://codereview.appspot.com/14642044 ...
8 years, 3 months ago (2013-10-15 23:01:42 UTC) #16
bradfitz
[+Tad] Tad, you might be interested in fixing this too, considering it affects your new ...
8 years, 3 months ago (2013-10-16 00:24:37 UTC) #17
Tad Glines
While closing a connection right after opening it might seems like a waste, doing so ...
8 years, 3 months ago (2013-10-16 03:31:20 UTC) #18
bradfitz
Sooner the better. The Go 1.2 window is closing soon. On Tue, Oct 15, 2013 ...
8 years, 3 months ago (2013-10-16 03:49:12 UTC) #19
Hierro
I've updated the CL with the smallest possible fix, albeit not the most efficient, as ...
8 years, 3 months ago (2013-10-16 12:04:59 UTC) #20
bketelsen
We're keen to see a fix for this applied. Anything we can do to help? ...
8 years, 3 months ago (2013-10-16 13:05:10 UTC) #21
Hierro
On 2013/10/16 13:05:10, bketelsen wrote: > We're keen to see a fix for this applied. ...
8 years, 3 months ago (2013-10-16 13:33:19 UTC) #22
Hierro
Small typo in previous email: step 2 should be hg clpatch 14611045 Regards, Alberto
8 years, 3 months ago (2013-10-16 13:34:13 UTC) #23
bradfitz
LGTM
8 years, 3 months ago (2013-10-16 16:22:27 UTC) #24
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=c67a1d0df694 *** database/sql: Fix connection leak and potential deadlock CL 10726044 introduced ...
8 years, 3 months ago (2013-10-16 16:23:02 UTC) #25
albert.strasheim
On 2013/10/16 16:23:02, bradfitz wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=c67a1d0df694 *** > database/sql: Fix connection ...
8 years, 3 months ago (2013-10-17 11:06:54 UTC) #26
Tad Glines
The problem is with the new test. Changing (in fakedb_test.go): if d.waitCh != nil { ...
8 years, 3 months ago (2013-10-17 13:28:14 UTC) #27
Hierro
8 years, 3 months ago (2013-10-17 14:18:38 UTC) #28
Message was sent while issue was closed.
On 2013/10/17 13:28:14, Tad Glines wrote:
> The problem is with the new test.
> Changing (in fakedb_test.go):
> 	if d.waitCh != nil {
> 		d.waitingCh <- struct{}{}
> 		<-d.waitCh
> 	}
> to
> 	if d.waitCh != nil {
> 		d.waitingCh <- struct{}{}
> 		<-d.waitCh
> 		d.waitCh = nil
> 		d.waitingCh = nil
> 	}
> 
> fixes it.

Thanks, I'll submit a CL right now.

Regards,
Alberto
Sign in to reply to this message.

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