code review 5759050: database/sql: ensure Stmts are correctly closed.

database/sql: ensure Stmts are correctly closed.

To make sure that there is no resource leak,
I suggest to fix the 'fakedb' driver such as it fails when any
Stmt is not closed.
First, add a check in fakeConn.Close().
Then, fix all missing Stmt.Close()/Rows.Close().
I am not sure that the strategy choose in fakeConn.Prepare/prepare* is ok.
The weak point in this patch is the change in Tx.Query:
  - Tests pass without this change,
  - I found it by manually analyzing the code,
  - I just try to make Tx.Query look like DB.Query.

[gwenn, 2012-03-06 08:12:34]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[bradfitz, 2012-03-06 23:00:43]

Gwenn,

This patch looks fine to me, but just so I understand: does this fix any
real bug?

I see it does fix a statement leak if there's an invalid Stmt.Query after
Prepare, which is good to fix, but minor.

And you say that the Tx change is "the weak point" in this patch... why?
 It seems fine.


On Tue, Mar 6, 2012 at 7:12 PM, <gwenn.kahz@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
>      database/sql: ensure Stmts are correctly closed.
>
> To make sure that there is no resource leak,
> I suggest to fix the 'fakedb' driver such as it fails when any
> Stmt is not closed.
> First, add a check in fakeConn.Close().
> Then, fix all missing Stmt.Close()/Rows.Close().
> I am not sure that the strategy choose in fakeConn.Prepare/prepare* is
> ok.
> The weak point in this patch is the change in Tx.Query:
>  - Tests pass without this change,
>  - I found it by manually analyzing the code,
>  - I just try to make Tx.Query look like DB.Query.
>
> Please review this at
http://codereview.appspot.com/**5759050/<http://codereview.appspot.com/5759050/>
>
> Affected files:
>  M src/pkg/database/sql/fakedb_**test.go
>  M src/pkg/database/sql/sql.go
>  M src/pkg/database/sql/sql_test.**go
>
>
> Index: src/pkg/database/sql/fakedb_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/fakedb_**test.go
> +++ b/src/pkg/database/sql/fakedb_**test.go
> @@ -213,6 +213,9 @@
>        if c.db == nil {
>                return errors.New("can't close; already closed")
>        }
> +       if c.stmtsMade > c.stmtsClosed {
> +               return errors.New("can't close; dangling statement(s)")
> +       }
>        c.db = nil
>        return nil
>  }
> @@ -249,6 +252,7 @@
>  //  just a limitation for fakedb)
>  func (c *fakeConn) prepareSelect(stmt *fakeStmt, parts []string)
> (driver.Stmt, error) {
>        if len(parts) != 3 {
> +               stmt.Close()
>                return nil, errf("invalid SELECT syntax with %d parts; want
> 3", len(parts))
>        }
>        stmt.table = parts[0]
> @@ -259,14 +263,17 @@
>                }
>                nameVal := strings.Split(colspec, "=")
>                if len(nameVal) != 2 {
> +                       stmt.Close()
>                        return nil, errf("SELECT on table %q has invalid
> column spec of %q (index %d)", stmt.table, colspec, n)
>                }
>                column, value := nameVal[0], nameVal[1]
>                _, ok := c.db.columnType(stmt.table, column)
>                if !ok {
> +                       stmt.Close()
>                        return nil, errf("SELECT on table %q references
> non-existent column %q", stmt.table, column)
>                }
>                if value != "?" {
> +                       stmt.Close()
>                        return nil, errf("SELECT on table %q has pre-bound
> value for where column %q; need a question mark",
>                                stmt.table, column)
>                }
> @@ -279,12 +286,14 @@
>  // parts are table|col=type,col2=type2
>  func (c *fakeConn) prepareCreate(stmt *fakeStmt, parts []string)
> (driver.Stmt, error) {
>        if len(parts) != 2 {
> +               stmt.Close()
>                return nil, errf("invalid CREATE syntax with %d parts; want
> 2", len(parts))
>        }
>        stmt.table = parts[0]
>        for n, colspec := range strings.Split(parts[1], ",") {
>                nameType := strings.Split(colspec, "=")
>                if len(nameType) != 2 {
> +                       stmt.Close()
>                        return nil, errf("CREATE table %q has invalid
> column spec of %q (index %d)", stmt.table, colspec, n)
>                }
>                stmt.colName = append(stmt.colName, nameType[0])
> @@ -296,17 +305,20 @@
>  // parts are table|col=?,col2=val
>  func (c *fakeConn) prepareInsert(stmt *fakeStmt, parts []string)
> (driver.Stmt, error) {
>        if len(parts) != 2 {
> +               stmt.Close()
>                return nil, errf("invalid INSERT syntax with %d parts; want
> 2", len(parts))
>        }
>        stmt.table = parts[0]
>        for n, colspec := range strings.Split(parts[1], ",") {
>                nameVal := strings.Split(colspec, "=")
>                if len(nameVal) != 2 {
> +                       stmt.Close()
>                        return nil, errf("INSERT table %q has invalid
> column spec of %q (index %d)", stmt.table, colspec, n)
>                }
>                column, value := nameVal[0], nameVal[1]
>                ctype, ok := c.db.columnType(stmt.table, column)
>                if !ok {
> +                       stmt.Close()
>                        return nil, errf("INSERT table %q references
> non-existent column %q", stmt.table, column)
>                }
>                stmt.colName = append(stmt.colName, column)
> @@ -322,10 +334,12 @@
>                        case "int32":
>                                i, err := strconv.Atoi(value)
>                                if err != nil {
> +                                       stmt.Close()
>                                        return nil, errf("invalid
> conversion to int32 from %q", value)
>                                }
>                                subsetVal = int64(i) // int64 is a subset
> type, but not int32
>                        default:
> +                               stmt.Close()
>                                return nil, errf("unsupported conversion
> for pre-bound parameter %q to type %q", value, ctype)
>                        }
>                        stmt.colValue = append(stmt.colValue, subsetVal)
> @@ -360,6 +374,7 @@
>        case "INSERT":
>                return c.prepareInsert(stmt, parts)
>        default:
> +               stmt.Close()
>                return nil, errf("unsupported command type %q", cmd)
>        }
>        return stmt, nil
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -561,9 +561,11 @@
>                return nil, err
>        }
>        rows, err := stmt.Query(args...)
> -       if err == nil {
> -               rows.closeStmt = stmt
> +       if err != nil {
> +               stmt.Close()
> +               return nil, err
>        }
> +       rows.closeStmt = stmt
>        return rows, err
>  }
>
> @@ -963,7 +965,7 @@
>        }
>
>        // TODO(bradfitz): for now we need to defensively clone all
> -       // []byte that the driver returned (not permitting
> +       // []byte that the driver returned (not permitting
>        // *RawBytes in Rows.Scan), since we're about to close
>        // the Rows in our defer, when we return from this function.
>        // the contract with the driver.Next(...) interface is that it
> Index: src/pkg/database/sql/sql_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql_**test.go
> +++ b/src/pkg/database/sql/sql_**test.go
> @@ -216,6 +216,7 @@
>        if err != nil {
>                t.Fatalf("Prepare: %v", err)
>        }
> +       defer stmt.Close()
>        var age int
>        for n, tt := range []struct {
>                name string
> @@ -256,6 +257,7 @@
>        if err != nil {
>                t.Errorf("Stmt, err = %v, %v", stmt, err)
>        }
> +       defer stmt.Close()
>
>        type execTest struct {
>                args    []interface{}
> @@ -297,11 +299,14 @@
>        if err != nil {
>                t.Fatalf("Stmt, err = %v, %v", stmt, err)
>        }
> +       defer stmt.Close()
>        tx, err := db.Begin()
>        if err != nil {
>                t.Fatalf("Begin = %v", err)
>        }
> -       _, err = tx.Stmt(stmt).Exec("Bobby", 7)
> +       txs := tx.Stmt(stmt)
> +       defer txs.Close()
> +       _, err = txs.Exec("Bobby", 7)
>        if err != nil {
>                t.Fatalf("Exec = %v", err)
>        }
> @@ -330,6 +335,7 @@
>        if err != nil {
>                t.Fatal(err)
>        }
> +       defer r.Close()
>
>        if !r.Next() {
>                if r.Err() != nil {
> @@ -510,6 +516,7 @@
>        if err != nil {
>                t.Fatalf("prepare: %v", err)
>        }
> +       defer stmt.Close()
>        if _, err := stmt.Exec(3, "chris", spec.rows[2].nullParam,
> spec.rows[2].notNullParam); err != nil {
>                t.Errorf("exec insert chris: %v", err)
>        }
>
>
>

[bradfitz, 2012-03-06 23:16:55]

Gwenn,

In particular, I tried to add your sqlite driver (github.com/gwenn/gosqlite)
to the external go-sql-test test suite (
https://github.com/bradfitz/go-sql-test), but it fails to compile:

$ go get github.com/gwenn/gosqlite
# github.com/gwenn/gosqlite
error: 'sqlite3_blob_reopen' undeclared (first use in this function)
error: (Each undeclared identifier is reported only once

Want to send me a pull request on github to add it there, once it's fixed?



On Wed, Mar 7, 2012 at 10:00 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Gwenn,
>
> This patch looks fine to me, but just so I understand: does this fix any
> real bug?
>
> I see it does fix a statement leak if there's an invalid Stmt.Query after
> Prepare, which is good to fix, but minor.
>
> And you say that the Tx change is "the weak point" in this patch... why?
>  It seems fine.
>
>
> On Tue, Mar 6, 2012 at 7:12 PM, <gwenn.kahz@gmail.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg
>>
>>
>> Description:
>>      database/sql: ensure Stmts are correctly closed.
>>
>> To make sure that there is no resource leak,
>> I suggest to fix the 'fakedb' driver such as it fails when any
>> Stmt is not closed.
>> First, add a check in fakeConn.Close().
>> Then, fix all missing Stmt.Close()/Rows.Close().
>> I am not sure that the strategy choose in fakeConn.Prepare/prepare* is
>> ok.
>> The weak point in this patch is the change in Tx.Query:
>>  - Tests pass without this change,
>>  - I found it by manually analyzing the code,
>>  - I just try to make Tx.Query look like DB.Query.
>>
>> Please review this at
http://codereview.appspot.com/**5759050/<http://codereview.appspot.com/5759050/>
>>
>> Affected files:
>>  M src/pkg/database/sql/fakedb_**test.go
>>  M src/pkg/database/sql/sql.go
>>  M src/pkg/database/sql/sql_test.**go
>>
>>
>> Index: src/pkg/database/sql/fakedb_**test.go
>> ==============================**==============================**=======
>> --- a/src/pkg/database/sql/fakedb_**test.go
>> +++ b/src/pkg/database/sql/fakedb_**test.go
>> @@ -213,6 +213,9 @@
>>        if c.db == nil {
>>                return errors.New("can't close; already closed")
>>        }
>> +       if c.stmtsMade > c.stmtsClosed {
>> +               return errors.New("can't close; dangling statement(s)")
>> +       }
>>        c.db = nil
>>        return nil
>>  }
>> @@ -249,6 +252,7 @@
>>  //  just a limitation for fakedb)
>>  func (c *fakeConn) prepareSelect(stmt *fakeStmt, parts []string)
>> (driver.Stmt, error) {
>>        if len(parts) != 3 {
>> +               stmt.Close()
>>                return nil, errf("invalid SELECT syntax with %d parts;
>> want 3", len(parts))
>>        }
>>        stmt.table = parts[0]
>> @@ -259,14 +263,17 @@
>>                }
>>                nameVal := strings.Split(colspec, "=")
>>                if len(nameVal) != 2 {
>> +                       stmt.Close()
>>                        return nil, errf("SELECT on table %q has invalid
>> column spec of %q (index %d)", stmt.table, colspec, n)
>>                }
>>                column, value := nameVal[0], nameVal[1]
>>                _, ok := c.db.columnType(stmt.table, column)
>>                if !ok {
>> +                       stmt.Close()
>>                        return nil, errf("SELECT on table %q references
>> non-existent column %q", stmt.table, column)
>>                }
>>                if value != "?" {
>> +                       stmt.Close()
>>                        return nil, errf("SELECT on table %q has pre-bound
>> value for where column %q; need a question mark",
>>                                stmt.table, column)
>>                }
>> @@ -279,12 +286,14 @@
>>  // parts are table|col=type,col2=type2
>>  func (c *fakeConn) prepareCreate(stmt *fakeStmt, parts []string)
>> (driver.Stmt, error) {
>>        if len(parts) != 2 {
>> +               stmt.Close()
>>                return nil, errf("invalid CREATE syntax with %d parts;
>> want 2", len(parts))
>>        }
>>        stmt.table = parts[0]
>>        for n, colspec := range strings.Split(parts[1], ",") {
>>                nameType := strings.Split(colspec, "=")
>>                if len(nameType) != 2 {
>> +                       stmt.Close()
>>                        return nil, errf("CREATE table %q has invalid
>> column spec of %q (index %d)", stmt.table, colspec, n)
>>                }
>>                stmt.colName = append(stmt.colName, nameType[0])
>> @@ -296,17 +305,20 @@
>>  // parts are table|col=?,col2=val
>>  func (c *fakeConn) prepareInsert(stmt *fakeStmt, parts []string)
>> (driver.Stmt, error) {
>>        if len(parts) != 2 {
>> +               stmt.Close()
>>                return nil, errf("invalid INSERT syntax with %d parts;
>> want 2", len(parts))
>>        }
>>        stmt.table = parts[0]
>>        for n, colspec := range strings.Split(parts[1], ",") {
>>                nameVal := strings.Split(colspec, "=")
>>                if len(nameVal) != 2 {
>> +                       stmt.Close()
>>                        return nil, errf("INSERT table %q has invalid
>> column spec of %q (index %d)", stmt.table, colspec, n)
>>                }
>>                column, value := nameVal[0], nameVal[1]
>>                ctype, ok := c.db.columnType(stmt.table, column)
>>                if !ok {
>> +                       stmt.Close()
>>                        return nil, errf("INSERT table %q references
>> non-existent column %q", stmt.table, column)
>>                }
>>                stmt.colName = append(stmt.colName, column)
>> @@ -322,10 +334,12 @@
>>                        case "int32":
>>                                i, err := strconv.Atoi(value)
>>                                if err != nil {
>> +                                       stmt.Close()
>>                                        return nil, errf("invalid
>> conversion to int32 from %q", value)
>>                                }
>>                                subsetVal = int64(i) // int64 is a subset
>> type, but not int32
>>                        default:
>> +                               stmt.Close()
>>                                return nil, errf("unsupported conversion
>> for pre-bound parameter %q to type %q", value, ctype)
>>                        }
>>                        stmt.colValue = append(stmt.colValue, subsetVal)
>> @@ -360,6 +374,7 @@
>>        case "INSERT":
>>                return c.prepareInsert(stmt, parts)
>>        default:
>> +               stmt.Close()
>>                return nil, errf("unsupported command type %q", cmd)
>>        }
>>        return stmt, nil
>> Index: src/pkg/database/sql/sql.go
>> ==============================**==============================**=======
>> --- a/src/pkg/database/sql/sql.go
>> +++ b/src/pkg/database/sql/sql.go
>> @@ -561,9 +561,11 @@
>>                return nil, err
>>        }
>>        rows, err := stmt.Query(args...)
>> -       if err == nil {
>> -               rows.closeStmt = stmt
>> +       if err != nil {
>> +               stmt.Close()
>> +               return nil, err
>>        }
>> +       rows.closeStmt = stmt
>>        return rows, err
>>  }
>>
>> @@ -963,7 +965,7 @@
>>        }
>>
>>        // TODO(bradfitz): for now we need to defensively clone all
>> -       // []byte that the driver returned (not permitting
>> +       // []byte that the driver returned (not permitting
>>        // *RawBytes in Rows.Scan), since we're about to close
>>        // the Rows in our defer, when we return from this function.
>>        // the contract with the driver.Next(...) interface is that it
>> Index: src/pkg/database/sql/sql_test.**go
>> ==============================**==============================**=======
>> --- a/src/pkg/database/sql/sql_**test.go
>> +++ b/src/pkg/database/sql/sql_**test.go
>> @@ -216,6 +216,7 @@
>>        if err != nil {
>>                t.Fatalf("Prepare: %v", err)
>>        }
>> +       defer stmt.Close()
>>        var age int
>>        for n, tt := range []struct {
>>                name string
>> @@ -256,6 +257,7 @@
>>        if err != nil {
>>                t.Errorf("Stmt, err = %v, %v", stmt, err)
>>        }
>> +       defer stmt.Close()
>>
>>        type execTest struct {
>>                args    []interface{}
>> @@ -297,11 +299,14 @@
>>        if err != nil {
>>                t.Fatalf("Stmt, err = %v, %v", stmt, err)
>>        }
>> +       defer stmt.Close()
>>        tx, err := db.Begin()
>>        if err != nil {
>>                t.Fatalf("Begin = %v", err)
>>        }
>> -       _, err = tx.Stmt(stmt).Exec("Bobby", 7)
>> +       txs := tx.Stmt(stmt)
>> +       defer txs.Close()
>> +       _, err = txs.Exec("Bobby", 7)
>>        if err != nil {
>>                t.Fatalf("Exec = %v", err)
>>        }
>> @@ -330,6 +335,7 @@
>>        if err != nil {
>>                t.Fatal(err)
>>        }
>> +       defer r.Close()
>>
>>        if !r.Next() {
>>                if r.Err() != nil {
>> @@ -510,6 +516,7 @@
>>        if err != nil {
>>                t.Fatalf("prepare: %v", err)
>>        }
>> +       defer stmt.Close()
>>        if _, err := stmt.Exec(3, "chris", spec.rows[2].nullParam,
>> spec.rows[2].notNullParam); err != nil {
>>                t.Errorf("exec insert chris: %v", err)
>>        }
>>
>>
>>
>

[gwenn, 2012-03-07 12:58:30]

Brad,
You are right when you say that this patch fix only a minor bug.
I was expecting to find the reason why your 'go-sql-test' fails with SQLite by
just adding the check in fakeConn.Close() but I was wrong (the error is
elsewhere (in Row.Scan)).

And the weak point is that there is no test associated with Tx.Query that can
validate the fix. I tried to keep the patch as small as possible and I am not
sure that I can actually write this test with my poor skill.

Regards.

On 2012/03/06 23:00:43, bradfitz wrote:
> Gwenn,
> 
> This patch looks fine to me, but just so I understand: does this fix any
> real bug?
> 
> I see it does fix a statement leak if there's an invalid Stmt.Query after
> Prepare, which is good to fix, but minor.
> 
> And you say that the Tx change is "the weak point" in this patch... why?
>  It seems fine.
> 
> 
> On Tue, Mar 6, 2012 at 7:12 PM, <mailto:gwenn.kahz@gmail.com> wrote:
> 
> > Reviewers: http://golang-dev_googlegroups.com,
> >
> > Message:
> > Hello mailto:golang-dev@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg
> >
> >
> > Description:
> >      database/sql: ensure Stmts are correctly closed.
> >
> > To make sure that there is no resource leak,
> > I suggest to fix the 'fakedb' driver such as it fails when any
> > Stmt is not closed.
> > First, add a check in fakeConn.Close().
> > Then, fix all missing Stmt.Close()/Rows.Close().
> > I am not sure that the strategy choose in fakeConn.Prepare/prepare* is
> > ok.
> > The weak point in this patch is the change in Tx.Query:
> >  - Tests pass without this change,
> >  - I found it by manually analyzing the code,
> >  - I just try to make Tx.Query look like DB.Query.
> >
> > Please review this at
>
http://codereview.appspot.com/**5759050/%3Chttp://codereview.appspot.com/5759...>
> >
> > Affected files:
> >  M src/pkg/database/sql/fakedb_**test.go
> >  M src/pkg/database/sql/sql.go
> >  M src/pkg/database/sql/sql_test.**go
> >
> >
> > Index: src/pkg/database/sql/fakedb_**test.go
> > ==============================**==============================**=======
> > --- a/src/pkg/database/sql/fakedb_**test.go
> > +++ b/src/pkg/database/sql/fakedb_**test.go
> > @@ -213,6 +213,9 @@
> >        if c.db == nil {
> >                return errors.New("can't close; already closed")
> >        }
> > +       if c.stmtsMade > c.stmtsClosed {
> > +               return errors.New("can't close; dangling statement(s)")
> > +       }
> >        c.db = nil
> >        return nil
> >  }
> > @@ -249,6 +252,7 @@
> >  //  just a limitation for fakedb)
> >  func (c *fakeConn) prepareSelect(stmt *fakeStmt, parts []string)
> > (driver.Stmt, error) {
> >        if len(parts) != 3 {
> > +               stmt.Close()
> >                return nil, errf("invalid SELECT syntax with %d parts; want
> > 3", len(parts))
> >        }
> >        stmt.table = parts[0]
> > @@ -259,14 +263,17 @@
> >                }
> >                nameVal := strings.Split(colspec, "=")
> >                if len(nameVal) != 2 {
> > +                       stmt.Close()
> >                        return nil, errf("SELECT on table %q has invalid
> > column spec of %q (index %d)", stmt.table, colspec, n)
> >                }
> >                column, value := nameVal[0], nameVal[1]
> >                _, ok := c.db.columnType(stmt.table, column)
> >                if !ok {
> > +                       stmt.Close()
> >                        return nil, errf("SELECT on table %q references
> > non-existent column %q", stmt.table, column)
> >                }
> >                if value != "?" {
> > +                       stmt.Close()
> >                        return nil, errf("SELECT on table %q has pre-bound
> > value for where column %q; need a question mark",
> >                                stmt.table, column)
> >                }
> > @@ -279,12 +286,14 @@
> >  // parts are table|col=type,col2=type2
> >  func (c *fakeConn) prepareCreate(stmt *fakeStmt, parts []string)
> > (driver.Stmt, error) {
> >        if len(parts) != 2 {
> > +               stmt.Close()
> >                return nil, errf("invalid CREATE syntax with %d parts; want
> > 2", len(parts))
> >        }
> >        stmt.table = parts[0]
> >        for n, colspec := range strings.Split(parts[1], ",") {
> >                nameType := strings.Split(colspec, "=")
> >                if len(nameType) != 2 {
> > +                       stmt.Close()
> >                        return nil, errf("CREATE table %q has invalid
> > column spec of %q (index %d)", stmt.table, colspec, n)
> >                }
> >                stmt.colName = append(stmt.colName, nameType[0])
> > @@ -296,17 +305,20 @@
> >  // parts are table|col=?,col2=val
> >  func (c *fakeConn) prepareInsert(stmt *fakeStmt, parts []string)
> > (driver.Stmt, error) {
> >        if len(parts) != 2 {
> > +               stmt.Close()
> >                return nil, errf("invalid INSERT syntax with %d parts; want
> > 2", len(parts))
> >        }
> >        stmt.table = parts[0]
> >        for n, colspec := range strings.Split(parts[1], ",") {
> >                nameVal := strings.Split(colspec, "=")
> >                if len(nameVal) != 2 {
> > +                       stmt.Close()
> >                        return nil, errf("INSERT table %q has invalid
> > column spec of %q (index %d)", stmt.table, colspec, n)
> >                }
> >                column, value := nameVal[0], nameVal[1]
> >                ctype, ok := c.db.columnType(stmt.table, column)
> >                if !ok {
> > +                       stmt.Close()
> >                        return nil, errf("INSERT table %q references
> > non-existent column %q", stmt.table, column)
> >                }
> >                stmt.colName = append(stmt.colName, column)
> > @@ -322,10 +334,12 @@
> >                        case "int32":
> >                                i, err := strconv.Atoi(value)
> >                                if err != nil {
> > +                                       stmt.Close()
> >                                        return nil, errf("invalid
> > conversion to int32 from %q", value)
> >                                }
> >                                subsetVal = int64(i) // int64 is a subset
> > type, but not int32
> >                        default:
> > +                               stmt.Close()
> >                                return nil, errf("unsupported conversion
> > for pre-bound parameter %q to type %q", value, ctype)
> >                        }
> >                        stmt.colValue = append(stmt.colValue, subsetVal)
> > @@ -360,6 +374,7 @@
> >        case "INSERT":
> >                return c.prepareInsert(stmt, parts)
> >        default:
> > +               stmt.Close()
> >                return nil, errf("unsupported command type %q", cmd)
> >        }
> >        return stmt, nil
> > Index: src/pkg/database/sql/sql.go
> > ==============================**==============================**=======
> > --- a/src/pkg/database/sql/sql.go
> > +++ b/src/pkg/database/sql/sql.go
> > @@ -561,9 +561,11 @@
> >                return nil, err
> >        }
> >        rows, err := stmt.Query(args...)
> > -       if err == nil {
> > -               rows.closeStmt = stmt
> > +       if err != nil {
> > +               stmt.Close()
> > +               return nil, err
> >        }
> > +       rows.closeStmt = stmt
> >        return rows, err
> >  }
> >
> > @@ -963,7 +965,7 @@
> >        }
> >
> >        // TODO(bradfitz): for now we need to defensively clone all
> > -       // []byte that the driver returned (not permitting
> > +       // []byte that the driver returned (not permitting
> >        // *RawBytes in Rows.Scan), since we're about to close
> >        // the Rows in our defer, when we return from this function.
> >        // the contract with the driver.Next(...) interface is that it
> > Index: src/pkg/database/sql/sql_test.**go
> > ==============================**==============================**=======
> > --- a/src/pkg/database/sql/sql_**test.go
> > +++ b/src/pkg/database/sql/sql_**test.go
> > @@ -216,6 +216,7 @@
> >        if err != nil {
> >                t.Fatalf("Prepare: %v", err)
> >        }
> > +       defer stmt.Close()
> >        var age int
> >        for n, tt := range []struct {
> >                name string
> > @@ -256,6 +257,7 @@
> >        if err != nil {
> >                t.Errorf("Stmt, err = %v, %v", stmt, err)
> >        }
> > +       defer stmt.Close()
> >
> >        type execTest struct {
> >                args    []interface{}
> > @@ -297,11 +299,14 @@
> >        if err != nil {
> >                t.Fatalf("Stmt, err = %v, %v", stmt, err)
> >        }
> > +       defer stmt.Close()
> >        tx, err := db.Begin()
> >        if err != nil {
> >                t.Fatalf("Begin = %v", err)
> >        }
> > -       _, err = tx.Stmt(stmt).Exec("Bobby", 7)
> > +       txs := tx.Stmt(stmt)
> > +       defer txs.Close()
> > +       _, err = txs.Exec("Bobby", 7)
> >        if err != nil {
> >                t.Fatalf("Exec = %v", err)
> >        }
> > @@ -330,6 +335,7 @@
> >        if err != nil {
> >                t.Fatal(err)
> >        }
> > +       defer r.Close()
> >
> >        if !r.Next() {
> >                if r.Err() != nil {
> > @@ -510,6 +516,7 @@
> >        if err != nil {
> >                t.Fatalf("prepare: %v", err)
> >        }
> > +       defer stmt.Close()
> >        if _, err := stmt.Exec(3, "chris", spec.rows[2].nullParam,
> > spec.rows[2].notNullParam); err != nil {
> >                t.Errorf("exec insert chris: %v", err)
> >        }
> >
> >
> >

[gwenn, 2012-03-07 13:19:58]

Brad,
As specified on github,
1) the build error is related to your sqlite version,
2) you can use the 'mattn' driver to reproduce the bug.
And I suggest you to keep only the 'mattn' driver. Mine is bloated with stuff
not exposed by the 'database/sql' package (like BLOB I/O)...

If I have enough time, I will try to write another patch to validate that a new
Stmt.Reset method is needed.
Regards.

On 2012/03/06 23:16:55, bradfitz wrote:
> Gwenn,
> 
> In particular, I tried to add your sqlite driver (github.com/gwenn/gosqlite)
> to the external go-sql-test test suite (
> https://github.com/bradfitz/go-sql-test), but it fails to compile:
> 
> $ go get github.com/gwenn/gosqlite
> # github.com/gwenn/gosqlite
> error: 'sqlite3_blob_reopen' undeclared (first use in this function)
> error: (Each undeclared identifier is reported only once
> 
> Want to send me a pull request on github to add it there, once it's fixed?

[gwenn, 2012-03-10 10:20:27]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gwenn, 2012-03-10 10:30:28]

A new test 'TestTxQueryInvalid' has been added to reproduce the minor bug in
Tx.Query:
"error closing DB: can't close; dangling statement(s)"
But another error is raised with the patch:
"error closing DB: can't close; already closed"
Sorry.

[gwenn, 2012-03-10 19:29:41]

Ok,
The same connection is released (ie putConn) twice:
1) by tx.Query() -> Stmt.Query() with err,
2) by defer tx.Rollback().
If I comment out the "s.db.putConn(ci, err)", all tests pass but it seems bad.

[bradfitz, 2012-03-10 19:33:28]

Yes, see the patch I just submitted with the fix.

Could you update this patch to apply to tip?
 On Mar 10, 2012 11:29 AM, <gwenn.kahz@gmail.com> wrote:

> Ok,
> The same connection is released (ie putConn) twice:
> 1) by tx.Query() -> Stmt.Query() with err,
> 2) by defer tx.Rollback().
> If I comment out the "s.db.putConn(ci, err)", all tests pass but it
> seems bad.
>
>
http://codereview.appspot.com/**5759050/<http://codereview.appspot.com/5759050/>
>

[gwenn, 2012-03-10 20:18:26]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gwenn, 2012-03-10 20:21:07]

Nice,
Your fix make all tests pass.
I have merged/updated the patch.
Thanks.

On Sat, Mar 10, 2012 at 8:33 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Yes, see the patch I just submitted with the fix.
>
> Could you update this patch to apply to tip?
>
> On Mar 10, 2012 11:29 AM, <gwenn.kahz@gmail.com> wrote:
>>
>> Ok,
>> The same connection is released (ie putConn) twice:
>> 1) by tx.Query() -> Stmt.Query() with err,
>> 2) by defer tx.Rollback().
>> If I comment out the "s.db.putConn(ci, err)", all tests pass but it
>> seems bad.
>>
>> http://codereview.appspot.com/5759050/

[bradfitz, 2012-03-10 22:13:12]

[+mattn.jp]

Mattn's sqlite driver still fails my tests, though:

ante:sqltest $ git remote -v
origin git@github.com:bradfitz/go-sql-test.git (fetch)
origin git@github.com:bradfitz/go-sql-test.git (push)

ante:sqltest $ pwd
/home/bradfitz/hack/go-sql-test/src/sqltest

ante:sqltest $ echo $GOPATH
/home/bradfitz/hack/go-sql-test

ante:sqltest $ go test -v -run=TestPreparedStmt_SQLite

=== RUN TestPreparedStmt_SQLite
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x2a pc=0x7f7e6f590840]

goroutine 1 [chan receive]:
testing.RunTests(0x400c00, 0x682070, 0xc0000000c, 0x1, 0xf800000007, ...)
/home/bradfitz/go/src/pkg/testing/testing.go:350 +0x79f
testing.Main(0x400c00, 0x682070, 0xc0000000c, 0x685480, 0x0, ...)
/home/bradfitz/go/src/pkg/testing/testing.go:285 +0x7a
main.main()
/tmp/go-build762216293/sqltest/_test/_testmain.go:65 +0x91

goroutine 2 [syscall]:
created by runtime.main
/home/bradfitz/go/src/pkg/runtime/proc.c:221

goroutine 3 [syscall]:
github.com/mattn/go-sqlite3._Cfunc_sqlite3_reset(0x3d07188, 0xf840092100)
/tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/_cgo_defun.c:218+0x2f
github.com/mattn/go-sqlite3.(*SQLiteStmt).bind(0xf840057c90, 0x6856e0, 0x0,
0x0, 0x0, ...)
/tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/sqlite3.cgo1.go:170+0x34
github.com/mattn/go-sqlite3.(*SQLiteStmt).Query(0xf840057c90, 0x6856e0,
0x0, 0x0, 0x0, ...)
/tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/sqlite3.cgo1.go:223+0x4a
database/sql.(*Stmt).Query(0xf8400588a0, 0x0, 0x0, 0x8, 0x100000001, ...)
/home/bradfitz/go/src/pkg/database/sql/sql.go:812 +0x28e
database/sql.(*Stmt).QueryRow(0xf8400588a0, 0x0, 0x0, 0xf840057c30,
0xf840063760, ...)
/home/bradfitz/go/src/pkg/database/sql/sql.go:840 +0x40
sqltest._func_004(0xf8400711a8, 0x7f7e6f995da8, 0x7f7e6f995e38,
0x7f7e6f995de8, 0xf840063700, ...)
/home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:301 +0x8e
sqltest.testPreparedStmt(0xf840057990, 0xf8400579c0)
/home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:310 +0x4db
sqltest.sqliteDB.RunTest(0xf840064230, 0x426e5a, 0x426dc3, 0xf8400579c0)
/home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:132 +0x361
sqltest.sqliteDB.RunTest·i(0xf8400579c0, 0xf840064230, 0x426e5a, 0x421b31,
0xf840064230, ...)
/home/bradfitz/hack/go-sql-test/src/sqltest/drivers.go:0 +0x38
sqltest.TestPreparedStmt_SQLite(0xf840064230, 0x1e965668)
/home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:271 +0x44
testing.tRunner(0xf840064230, 0x682148, 0x0, 0x0)
/home/bradfitz/go/src/pkg/testing/testing.go:273 +0x6f
created by testing.RunTests
/home/bradfitz/go/src/pkg/testing/testing.go:349 +0x77c
exit status 2
FAIL sqltest 0.022s

Or, sometimes it fails like:

--- FAIL: TestPreparedStmt_SQLite (0.02 seconds)
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:302: Query: sql: statement expects 1117 inputs; got 0


Or sometimes like:

--- FAIL: TestPreparedStmt_SQLite (0.05 seconds)
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:306: Insert: database is locked
sql_test.go:302: Query: SQL logic error or missing database
FAIL


I could use help debugging why the mattn driver fails like this.  I'll keep
digging, but more eyeballs welcome.

- Brad


On Sat, Mar 10, 2012 at 12:21 PM, gwenn <gwenn.kahz@gmail.com> wrote:

> Nice,
> Your fix make all tests pass.
> I have merged/updated the patch.
> Thanks.
>
> On Sat, Mar 10, 2012 at 8:33 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Yes, see the patch I just submitted with the fix.
> >
> > Could you update this patch to apply to tip?
> >
> > On Mar 10, 2012 11:29 AM, <gwenn.kahz@gmail.com> wrote:
> >>
> >> Ok,
> >> The same connection is released (ie putConn) twice:
> >> 1) by tx.Query() -> Stmt.Query() with err,
> >> 2) by defer tx.Rollback().
> >> If I comment out the "s.db.putConn(ci, err)", all tests pass but it
> >> seems bad.
> >>
> >> http://codereview.appspot.com/5759050/
>

[bradfitz, 2012-03-10 23:18:58]

LGTM

On Sat, Mar 10, 2012 at 12:21 PM, gwenn <gwenn.kahz@gmail.com> wrote:

> Nice,
> Your fix make all tests pass.
> I have merged/updated the patch.
> Thanks.
>
> On Sat, Mar 10, 2012 at 8:33 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Yes, see the patch I just submitted with the fix.
> >
> > Could you update this patch to apply to tip?
> >
> > On Mar 10, 2012 11:29 AM, <gwenn.kahz@gmail.com> wrote:
> >>
> >> Ok,
> >> The same connection is released (ie putConn) twice:
> >> 1) by tx.Query() -> Stmt.Query() with err,
> >> 2) by defer tx.Rollback().
> >> If I comment out the "s.db.putConn(ci, err)", all tests pass but it
> >> seems bad.
> >>
> >> http://codereview.appspot.com/5759050/
>

[bradfitz, 2012-03-10 23:25:14]

*** Submitted as http://code.google.com/p/go/source/detail?r=91a86970157c ***

database/sql: ensure Stmts are correctly closed.

To make sure that there is no resource leak,
I suggest to fix the 'fakedb' driver such as it fails when any
Stmt is not closed.
First, add a check in fakeConn.Close().
Then, fix all missing Stmt.Close()/Rows.Close().
I am not sure that the strategy choose in fakeConn.Prepare/prepare* is ok.
The weak point in this patch is the change in Tx.Query:
  - Tests pass without this change,
  - I found it by manually analyzing the code,
  - I just try to make Tx.Query look like DB.Query.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5759050

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[gwenn, 2012-03-11 11:34:09]

Hello,
First, I think that Rows.Close() implementation should be fixed: the
associated Stmt must be Reset (and not Closed).

But with this fix, the go-sql-test still fails:
2012/03/11 12:23:14 SQLITE: Library used incorrectly, Dangling
statement (not finalize): "INSERT INTO t (count) VALUES (?)"
...
2012/03/11 12:23:14 SQLITE: Library used incorrectly, Dangling
statement (not finalize): "SELECT count FROM t ORDER BY count DESC"
...
2012/03/11 12:23:14 SQLITE: Library used incorrectly, Dangling
statement (not finalize): ""
panic: runtime error: invalid memory address or nil pointer dereference
...
I am investigating.
But I am stupid/slow: how do you easily have a backtrace in Go?
Regards.

On Sat, Mar 10, 2012 at 11:13 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> [+mattn.jp]
>
> Mattn's sqlite driver still fails my tests, though:
>
> ante:sqltest $ git remote -v
> origin git@github.com:bradfitz/go-sql-test.git (fetch)
> origin git@github.com:bradfitz/go-sql-test.git (push)
>
> ante:sqltest $ pwd
> /home/bradfitz/hack/go-sql-test/src/sqltest
>
> ante:sqltest $ echo $GOPATH
> /home/bradfitz/hack/go-sql-test
>
> ante:sqltest $ go test -v -run=TestPreparedStmt_SQLite
>
> === RUN TestPreparedStmt_SQLite
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x2a pc=0x7f7e6f590840]
>
> goroutine 1 [chan receive]:
> testing.RunTests(0x400c00, 0x682070, 0xc0000000c, 0x1, 0xf800000007, ...)
> /home/bradfitz/go/src/pkg/testing/testing.go:350 +0x79f
> testing.Main(0x400c00, 0x682070, 0xc0000000c, 0x685480, 0x0, ...)
> /home/bradfitz/go/src/pkg/testing/testing.go:285 +0x7a
> main.main()
> /tmp/go-build762216293/sqltest/_test/_testmain.go:65 +0x91
>
> goroutine 2 [syscall]:
> created by runtime.main
> /home/bradfitz/go/src/pkg/runtime/proc.c:221
>
> goroutine 3 [syscall]:
> github.com/mattn/go-sqlite3._Cfunc_sqlite3_reset(0x3d07188, 0xf840092100)
> /tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/_cgo_defun.c:218
> +0x2f
> github.com/mattn/go-sqlite3.(*SQLiteStmt).bind(0xf840057c90, 0x6856e0, 0x0,
> 0x0, 0x0, ...)
> /tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/sqlite3.cgo1.go:170
> +0x34
> github.com/mattn/go-sqlite3.(*SQLiteStmt).Query(0xf840057c90, 0x6856e0, 0x0,
> 0x0, 0x0, ...)
> /tmp/go-build762216293/github.com/mattn/go-sqlite3/_obj/sqlite3.cgo1.go:223
> +0x4a
> database/sql.(*Stmt).Query(0xf8400588a0, 0x0, 0x0, 0x8, 0x100000001, ...)
> /home/bradfitz/go/src/pkg/database/sql/sql.go:812 +0x28e
> database/sql.(*Stmt).QueryRow(0xf8400588a0, 0x0, 0x0, 0xf840057c30,
> 0xf840063760, ...)
> /home/bradfitz/go/src/pkg/database/sql/sql.go:840 +0x40
> sqltest._func_004(0xf8400711a8, 0x7f7e6f995da8, 0x7f7e6f995e38,
> 0x7f7e6f995de8, 0xf840063700, ...)
> /home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:301 +0x8e
> sqltest.testPreparedStmt(0xf840057990, 0xf8400579c0)
> /home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:310 +0x4db
> sqltest.sqliteDB.RunTest(0xf840064230, 0x426e5a, 0x426dc3, 0xf8400579c0)
> /home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:132 +0x361
> sqltest.sqliteDB.RunTest·i(0xf8400579c0, 0xf840064230, 0x426e5a, 0x421b31,
> 0xf840064230, ...)
> /home/bradfitz/hack/go-sql-test/src/sqltest/drivers.go:0 +0x38
> sqltest.TestPreparedStmt_SQLite(0xf840064230, 0x1e965668)
> /home/bradfitz/hack/go-sql-test/src/sqltest/sql_test.go:271 +0x44
> testing.tRunner(0xf840064230, 0x682148, 0x0, 0x0)
> /home/bradfitz/go/src/pkg/testing/testing.go:273 +0x6f
> created by testing.RunTests
> /home/bradfitz/go/src/pkg/testing/testing.go:349 +0x77c
> exit status 2
> FAIL sqltest 0.022s
>
> Or, sometimes it fails like:
>
> --- FAIL: TestPreparedStmt_SQLite (0.02 seconds)
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:302: Query: sql: statement expects 1117 inputs; got 0
>
>
> Or sometimes like:
>
> --- FAIL: TestPreparedStmt_SQLite (0.05 seconds)
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:306: Insert: database is locked
> sql_test.go:302: Query: SQL logic error or missing database
> FAIL
>
>
> I could use help debugging why the mattn driver fails like this.  I'll keep
> digging, but more eyeballs welcome.
>
> - Brad
>
>
> On Sat, Mar 10, 2012 at 12:21 PM, gwenn <gwenn.kahz@gmail.com> wrote:
>>
>> Nice,
>> Your fix make all tests pass.
>> I have merged/updated the patch.
>> Thanks.
>>
>> On Sat, Mar 10, 2012 at 8:33 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > Yes, see the patch I just submitted with the fix.
>> >
>> > Could you update this patch to apply to tip?
>> >
>> > On Mar 10, 2012 11:29 AM, <gwenn.kahz@gmail.com> wrote:
>> >>
>> >> Ok,
>> >> The same connection is released (ie putConn) twice:
>> >> 1) by tx.Query() -> Stmt.Query() with err,
>> >> 2) by defer tx.Rollback().
>> >> If I comment out the "s.db.putConn(ci, err)", all tests pass but it
>> >> seems bad.
>> >>
>> >> http://codereview.appspot.com/5759050/
>
>