code review 14642044: database/sql: Fix connection leaks and numOpen count

database/sql: Fix connection leaks and numOpen count

Add a check at the end of every test to make sure
there are no leaked connections after running a test.

Avoid incorrectly decrementing the number of open connections when the driver connection ends up it a bad state (numOpen was decremented twice).

Prevent leaking a Rows struct (which ends up leaking a connection) in Row.Scan() when a *RawBytes destination is used.

Close the Rows struct in TestRowsColumns.

[Hierro, 2013-10-15 23:01:07]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-10-15 23:47:13]

Is this related to your previous CL at all?  There is no bug number in this
CL, or description of what it fixes.



On Tue, Oct 15, 2013 at 1:01 PM, <alberto.garcia.hierro@gmail.com> 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 leaks numOpen count
>
> Add also a check at the end of every test to make sure
> there are no connections left open.
>
> Please review this at
https://codereview.appspot.**com/14642044/<https://codereview.appspot.com/146...
>
> Affected files (+10, -3 lines):
>   M src/pkg/database/sql/sql.go
>   M src/pkg/database/sql/sql_test.**go
>
>
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -743,8 +743,8 @@
>         if err == driver.ErrBadConn {
>                 // Don't reuse bad connections.
>                 // Since the conn is considered bad and is being
> discarded, treat it
> -               // as closed. Decrement the open count.
> -               db.numOpen--
> +               // as closed. Don't decrement the open count here,
> finalClose will
> +               // take care of that.
>                 db.maybeOpenNewConnections()
>                 db.mu.Unlock()
>                 dc.Close()
> @@ -1607,13 +1607,13 @@
>         // from Next will not be modified again." (for instance, if
>         // they were obtained from the network anyway) But for now we
>         // don't care.
> +       defer r.rows.Close()
>         for _, dp := range dest {
>                 if _, ok := dp.(*RawBytes); ok {
>                         return errors.New("sql: RawBytes isn't allowed on
> Row.Scan")
>                 }
>         }
>
> -       defer r.rows.Close()
>         if !r.rows.Next() {
>                 return ErrNoRows
>         }
> Index: src/pkg/database/sql/sql_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql_**test.go
> +++ b/src/pkg/database/sql/sql_**test.go
> @@ -94,6 +94,10 @@
>         if err != nil {
>                 t.Fatalf("error closing DB: %v", err)
>         }
> +       runtime.GC()
> +       if db.numOpen != 0 {
> +               t.Fatalf("%d connections still open after closing DB",
> db.numOpen)
> +       }
>  }
>
>  // numPrepares assumes that db has exactly 1 idle conn and returns
> @@ -246,6 +250,9 @@
>         if !reflect.DeepEqual(cols, want) {
>                 t.Errorf("got %#v; want %#v", cols, want)
>         }
> +       if err := rows.Close(); err != nil {
> +               t.Errorf("error closing rows: %s", err)
> +       }
>  }
>
>  func TestQueryRow(t *testing.T) {
>
>
> --
>
> ---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@googlegrou...
> .
> To view this discussion on the web visit https://groups.google.com/d/**
>
msgid/golang-dev/**089e0160a2b85ccb7c04e8cf8f54%**40google.com<https://groups.google.com/d/msgid/golang-dev/089e0160a2b85ccb7c04e8cf8f54%40google.com>
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

[bradfitz, 2013-10-16 00:12:20]

There is a data race with this fix.

=== RUN TestStmtCloseOrder
==================
WARNING: DATA RACE
Read by goroutine 127:
  database/sql.closeDB()
      /Users/bradfitz/go/src/pkg/database/sql/sql_test.go:98 +0x643
  database/sql.TestStmtCloseOrder()
      /Users/bradfitz/go/src/pkg/database/sql/sql_test.go:1225 +0x225
  testing.tRunner()
      /Users/bradfitz/go/src/pkg/testing/testing.go:389 +0x10f

Previous write by goroutine 129:
  database/sql.(*driverConn).finalClose()
      /Users/bradfitz/go/src/pkg/database/sql/sql.go:312 +0x209
  database/sql.finalCloser.(database/sql.finalClose)·fm()
      /Users/bradfitz/go/src/pkg/database/sql/sql.go:394 +0x4d
  database/sql.(*driverConn).Close()
      /Users/bradfitz/go/src/pkg/database/sql/sql.go:295 +0x24b

Goroutine 127 (running) created at:
  testing.RunTests()
      /Users/bradfitz/go/src/pkg/testing/testing.go:469 +0xb3c
  testing.Main()
      /Users/bradfitz/go/src/pkg/testing/testing.go:401 +0xa2
  main.main()
      database/sql/_test/_testmain.go:135 +0xdc

Goroutine 129 (finished) created at:
  database/sql.(*DB).SetMaxIdleConns()
      /Users/bradfitz/go/src/pkg/database/sql/sql.go:522 +0x2bf
  database/sql.TestStmtCloseOrder()
      /Users/bradfitz/go/src/pkg/database/sql/sql_test.go:1217 +0xd9
  testing.tRunner()
      /Users/bradfitz/go/src/pkg/testing/testing.go:389 +0x10f
==================

[Hierro, 2013-10-16 00:58:06]

Hi Brad,

Nice catch. I didn't realize the tests used the DB concurrently, so I didn't put
Lock/Unlock around the check. That should be fixed now. This CL is related to
the another one because it also fixes connection leaks. The new check at the end
of every test will help us catching leaks in the future. There's no bug number
because I didn't think opening a issue for these small fixes was worth it.

I've also documented exactly what I've changed and why in the CL description (I
thought these small changes were self explanatory, but you're right that the CL
description should be more detailed than it was).

PTAL

Regards,
Alberto

P.S: Going to bed, won't be able to reply until ~8 hours from now.

[dfc, 2013-10-16 01:28:01]

https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
File src/pkg/database/sql/sql_test.go (right):

https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
src/pkg/database/sql/sql_test.go:93: err := db.Close()
if err := ... ; err != nil {

https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
src/pkg/database/sql/sql_test.go:97: runtime.GC()
What does this call to runtime.GC() do ?

[Hierro, 2013-10-16 11:53:20]

On 2013/10/16 01:28:01, dfc wrote:
>
https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
> File src/pkg/database/sql/sql_test.go (right):
> 
>
https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
> src/pkg/database/sql/sql_test.go:93: err := db.Close()
> if err := ... ; err != nil {

That's a cosmetic change. I'm not going to mix fixes with cosmetic changes in
the same CL.

> 
>
https://codereview.appspot.com/14642044/diff/12001/src/pkg/database/sql/sql_t...
> src/pkg/database/sql/sql_test.go:97: runtime.GC()
> What does this call to runtime.GC() do ?

That was to make the goroutine yield, because the code in SetMaxIdleConns()
closes connections in goroutines. That wasn't very elegant, so I've updated the
code to close the connections without spawning new goroutines.

PTAL

Regards,
Alberto

[bradfitz, 2013-10-16 16:09:35]

LGTM

Thanks!

[bradfitz, 2013-10-16 16:17:37]

*** Submitted as https://code.google.com/p/go/source/detail?r=bafb49a30d5f ***

database/sql: fix double decrement of numOpen count; test for connection leaks

Add a check at the end of every test to make sure
there are no leaked connections after running a test.

Avoid incorrectly decrementing the number of open connections
when the driver connection ends up it a bad state (numOpen was
decremented twice).

Prevent leaking a Rows struct (which ends up leaking a
connection) in Row.Scan() when a *RawBytes destination is
improperly used.

Close the Rows struct in TestRowsColumns.

Update issue 6593

R=golang-dev, bradfitz, dave
CC=golang-dev
https://codereview.appspot.com/14642044

Committer: Brad Fitzpatrick <bradfitz@golang.org>