Data race in thrsafe.go

[chrhlnd]

Last time is being accessed and written too without a barrier. I had it spam me in testing.

diff --git a/thrsafe/thrsafe.go b/thrsafe/thrsafe.go
index 00e05f6..3979bb9 100644
--- a/thrsafe/thrsafe.go
+++ b/thrsafe/thrsafe.go
@@ -21,7 +21,7 @@ type Conn struct {
        mutex *sync.Mutex

        stopPinger chan struct{}
-       lastUsed   time.Time
+       lastUsed   chan time.Time
 }

 func (c *Conn) lock() {
@@ -31,7 +31,7 @@ func (c *Conn) lock() {

 func (c *Conn) unlock() {
        //log.Println(c, ":: unlock @", c.mutex)
-       c.lastUsed = time.Now()
+       c.lastUsed <- time.Now()
        c.mutex.Unlock()
 }

@@ -67,13 +67,17 @@ func (c *Conn) Clone() mysql.Conn {
 func (c *Conn) pinger() {
        const to = 60 * time.Second
        sleep := to
+       var lastTime time.Time
        for {
                timer := time.After(sleep)
                select {
                case <-c.stopPinger:
                        return
+               case t := <-c.lastUsed:
+                       lastTime = t
+                       break
                case t := <-timer:
-                       sleep := to - t.Sub(c.lastUsed)
+                       sleep := to - t.Sub(lastTime)
                        if sleep <= 0 {
                                if c.Ping() != nil {
                                        return
@@ -283,7 +287,7 @@ func (c *Conn) Begin() (mysql.Transaction, error) {
        //log.Println("Begin")
        c.lock()
        tr := Transaction{
-               &Conn{Conn: c.Conn, mutex: new(sync.Mutex)},
+               &Conn{Conn: c.Conn, mutex: new(sync.Mutex), lastUsed: make(chan time.Time)},
                c,
        }
        _, err := c.Conn.Start("START TRANSACTION")

[ziutek]

LGTM.

Can you fill this patch as pull request?

[binary132]

Was this ever resolved?

[chrhlnd]

Haven't had time to issue a patch on this. This small patch here isn't completely correct eaither, due to lastUsed being allocated somewhere else (last I remember) and being defaulted to nil. When the other allocation is used then the connection will hang because sending on a nil channel does that.

A better patch would be to rename the variable to get the compiler to list the locations of it, then make sure the channel is always allocated then do some testing.