code review 12677050: database/sql: make Rows.Next returning false always imp...

database/sql: make Rows.Next returning false always implicitly call
Rows.Close.

Previously, callers that followed the example code (but not call
rows.Close after "for rows.Next() { ... }") could leak statements if
the driver returned an error other than io.EOF.

[nigeltao, 2013-08-12 07:05:29]

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

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

[nigeltao, 2013-08-12 07:16:19]

"go test database/sql" passes but that doesn't actually exercise any of the real
drivers. How else should I test this?

I'm also not overly familiar with database/sql so I don't know if this will
break any existing drivers. Who should be CC'ed on this?

[brainman, 2013-08-12 07:35:07]

Brad will want new test. :-)

I tested this against code.google.com/p/odbc and it works OK. I am not familiar
enough with this code myself, so I don't know if your change is good. Better
wait for brad.

Alex

[bradfitz, 2013-08-12 22:11:00]

Seems fine.  I can't see this breaking anything, but it'd be nice to add a test.

I just modify the in-memory fake driver (in fakedb_test.go) to make it do what I
want, or add hooks in the sql.go code to inject failures, etc.

https://codereview.appspot.com/12677050/diff/6001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/12677050/diff/6001/src/pkg/database/sql/sql.go...
src/pkg/database/sql/sql.go:1373: // result of Err. Close is idempotent and does
not affect the result of Err.
> Close is idempotent and does not affect the result of Err.

Except that part where you assign to rs.err.  So not really.

[nigeltao, 2013-08-14 01:25:18]

Test added. PTAL.

https://codereview.appspot.com/12677050/diff/6001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/12677050/diff/6001/src/pkg/database/sql/sql.go...
src/pkg/database/sql/sql.go:1373: // result of Err. Close is idempotent and does
not affect the result of Err.
On 2013/08/12 22:11:00, bradfitz wrote:
> > Close is idempotent and does not affect the result of Err.
> 
> Except that part where you assign to rs.err.  So not really.

Code reverted to match documentation.

[bradfitz, 2013-08-14 20:44:11]

LGTM

thanks!

[nigeltao, 2013-08-16 01:23:51]

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

database/sql: make Rows.Next returning false always implicitly call
Rows.Close.

Previously, callers that followed the example code (but not call
rows.Close after "for rows.Next() { ... }") could leak statements if
the driver returned an error other than io.EOF.

R=bradfitz, alex.brainman
CC=golang-dev, rsc
https://codereview.appspot.com/12677050