code review 7803043: database/sql: associate a mutex with each driver interface

database/sql: associate a mutex with each driver interface

The database/sql/driver docs make this promise:

   "Conn is a connection to a database. It is not used
   concurrently by multiple goroutines."

That promises exists as part of database/sql's overall
goal of making drivers relatively easy to write.

So far this promise has been kept without the use of locks by
being careful in the database/sql package, but sometimes too
careful. (cf. golang.org/issue/3857)

The CL associates a Mutex with each driver.Conn, and with the
interface value progeny thereof. (e.g. each driver.Tx,
driver.Stmt, driver.Rows, driver.Result, etc) Then whenever
those interface values are used, the Locker is locked.

This CL should be a no-op (aside from some new Lock/Unlock
pairs) and doesn't attempt to fix Issue 3857 or Issue 4459,
but should make it much easier in a subsequent CL.

Update issue 3857

[bradfitz, 2013-03-13 22:52:39]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-03-13 22:55:44]

This CL probably looks scarier than it is, due to some variable renaming.

For example.

var ci driver.Conn  (mnemonic: "conn interface")
->
var dc *driverConn (which now has a ".ci" member)

Likewise with driverResult, driverStmt.

On Wed, Mar 13, 2013 at 3:52 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> database/sql: associate a mutex with each driver interface
>
> The database/sql/driver docs make this promise:
>
>    "Conn is a connection to a database. It is not used
>    concurrently by multiple goroutines."
>
> That promises exists as part of database/sql's overall
> goal of making drivers relatively easy to write.
>
> So far this promise has been kept without the use of locks by
> being careful in the database/sql package, but sometimes too
> careful. (cf. golang.org/issue/3857)
>
> The CL associates a Mutex with each driver.Conn, and with the
> interface value progeny thereof. (e.g. each driver.Tx,
> driver.Stmt, driver.Rows, driver.Result, etc) Then whenever
> those interface values are used, the Locker is locked.
>
> This CL should be a no-op (aside from some new Lock/Unlock
> pairs) and doesn't attempt to fix Issue 3857 or Issue 4459,
> but should make it much easier in a subsequent CL.
>
> Update issue 3857
>
> Please review this at
https://codereview.appspot.**com/7803043/<https://codereview.appspot.com/7803...
>
> Affected files:
>   M src/pkg/database/sql/convert.**go
>   M src/pkg/database/sql/sql.go
>   M src/pkg/database/sql/sql_test.**go
>
>
>

[adg, 2013-03-14 00:53:43]

I'm wondering whether you should be using defer to unlock before calling into
driver-implemented functions. Does it matter if a panic happens and doesn't
unlock the conn? I guess the conn is still hosed in that case anyway.

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

https://codereview.appspot.com/7803043/diff/6/src/pkg/database/sql/sql.go#new...
src/pkg/database/sql/sql.go:222: ds.Unlock()
defer?

https://codereview.appspot.com/7803043/diff/6/src/pkg/database/sql/sql.go#new...
src/pkg/database/sql/sql.go:440: c.Unlock()
defer?

https://codereview.appspot.com/7803043/diff/6/src/pkg/database/sql/sql.go#new...
src/pkg/database/sql/sql.go:602: withLock(dc, func() { si.Close() })
why do you use withLock here?, and not above?

[bradfitz, 2013-03-14 00:55:30]

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

https://codereview.appspot.com/7803043/diff/6/src/pkg/database/sql/sql.go#new...
src/pkg/database/sql/sql.go:602: withLock(dc, func() { si.Close() })
On 2013/03/14 00:53:43, adg wrote:
> why do you use withLock here?, and not above?

I use withLock when I don't look at the return values, because withLock doesn't
return anthing.  Mostly with Close.

[bradfitz, 2013-03-14 01:17:14]

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

Please take another look.

[adg, 2013-03-14 02:13:27]

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

https://codereview.appspot.com/7803043/diff/11001/src/pkg/database/sql/sql.go...
src/pkg/database/sql/sql.go:1330: func withLock(lk sync.Locker, fn func()) {
I don't think this is really worth the trouble.

this is 32 chars:
withLock(a, func() { call() })

this is 27:
a.Lock()
call()
a.Unlock()

[bradfitz, 2013-03-14 03:15:48]

I added withLock just for brevity of the defers where it's shorter. Then I
thought you wanted me to use it more.
On Mar 13, 2013 7:13 PM, <adg@golang.org> wrote:

>
> https://codereview.appspot.**com/7803043/diff/11001/src/**
>
pkg/database/sql/sql.go<https://codereview.appspot.com/7803043/diff/11001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> https://codereview.appspot.**com/7803043/diff/11001/src/**
>
pkg/database/sql/sql.go#**newcode1330<https://codereview.appspot.com/7803043/diff/11001/src/pkg/database/sql/sql.go#newcode1330>
> src/pkg/database/sql/sql.go:**1330: func withLock(lk sync.Locker, fn
> func()) {
> I don't think this is really worth the trouble.
>
> this is 32 chars:
> withLock(a, func() { call() })
>
> this is 27:
> a.Lock()
> call()
> a.Unlock()
>
>
https://codereview.appspot.**com/7803043/<https://codereview.appspot.com/7803...
>

[adg, 2013-03-14 03:31:04]

On 14 March 2013 14:15, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> I added withLock just for brevity of the defers where it's shorter. Then I
> thought you wanted me to use it more.


I did when I thought it added something, but then I realized it didn't and
just made the code a bit harder to read.

Sorry for the change of heart. I'm not trying to make this difficult, I
swear!

[bradfitz, 2013-03-14 16:55:54]

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

Please take another look.

[bradfitz, 2013-03-14 16:56:09]

Okay, re-uploaded only using withLock for defers.

On Wed, Mar 13, 2013 at 8:30 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 14 March 2013 14:15, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>> I added withLock just for brevity of the defers where it's shorter. Then
>> I thought you wanted me to use it more.
>
>
> I did when I thought it added something, but then I realized it didn't and
> just made the code a bit harder to read.
>
> Sorry for the change of heart. I'm not trying to make this difficult, I
> swear!
>

[adg, 2013-03-14 21:52:01]

LGTM

[bradfitz, 2013-03-14 22:01:48]

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

database/sql: associate a mutex with each driver interface

The database/sql/driver docs make this promise:

   "Conn is a connection to a database. It is not used
   concurrently by multiple goroutines."

That promises exists as part of database/sql's overall
goal of making drivers relatively easy to write.

So far this promise has been kept without the use of locks by
being careful in the database/sql package, but sometimes too
careful. (cf. golang.org/issue/3857)

The CL associates a Mutex with each driver.Conn, and with the
interface value progeny thereof. (e.g. each driver.Tx,
driver.Stmt, driver.Rows, driver.Result, etc) Then whenever
those interface values are used, the Locker is locked.

This CL should be a no-op (aside from some new Lock/Unlock
pairs) and doesn't attempt to fix Issue 3857 or Issue 4459,
but should make it much easier in a subsequent CL.

Update issue 3857

R=golang-dev, adg
CC=golang-dev
https://codereview.appspot.com/7803043