Driver: invalid connection

[lintanghui]

Issue description

Func (mc *mysqlConn)watchCancel() set var watching to true when conn request timeout and return err context deadline exceeded, exec mc.cleanup() to close the connection.but in standard lib, only when err equal to ErrBadConn ,the conn would be closed. Otherwise, err would be ignored and connection will be put back into conn pool.Next time I get connection from conn pool, I would get a bad connection which had been close by driver because of err deadline exceeded.

Example code

In go-sql-driver

func (mc *mysqlConn) watchCancel(ctx context.Context) error {
	if mc.watching {
		// Reach here if canceled,
		// so the connection is already invalid
		mc.cleanup()
		return nil
	}
	if ctx.Done() == nil {
		return nil
	}
// watching would be set to true if ctx.Done() return err, and mc.cleanup() would to exec next time 
// connection would be closed  .
	mc.watching = true
	select {
	default:
	case <-ctx.Done():
		return ctx.Err()
	}
	if mc.watcher == nil {
		return nil
	}

	mc.watcher <- ctx

	return nil
}

But in standard Lib. function putConn would put conn back to pool unless err equal to ErrBadConn:
https://github.com/golang/go/blob/release-branch.go1.8/src/database/sql/sql.go#L1032.
Otherwise, err would be ignored at https://github.com/golang/go/blob/release-branch.go1.8/src/database/sql/sql.go#L1045 and conn would be put back into pool.

func (db *DB) putConn(dc *driverConn, err error) {
	db.mu.Lock()
	if !dc.inUse {
		if debugGetPut {
			fmt.Printf("putConn(%v) DUPLICATE was: %s\n\nPREVIOUS was: %s", dc, stack(), db.lastPut[dc])
		}
		panic("sql: connection returned that was never out")
	}
	if debugGetPut {
		db.lastPut[dc] = stack()
	}
	dc.inUse = false

	for _, fn := range dc.onPut {
		fn()
	}
	dc.onPut = nil
  // Here, only if err equal to ErrBadConn conn would be closed
	if err == driver.ErrBadConn {
		// Don't reuse bad connections.
		// Since the conn is considered bad and is being discarded, treat it
		// as closed. Don't decrement the open count here, finalClose will
		// take care of that.
		db.maybeOpenNewConnections()
		db.mu.Unlock()
		dc.Close()
		return
	}
	if putConnHook != nil {
		putConnHook(db, dc)
	}
 // Here, conn would be put back into pool.next time i get conn from pool, i would get a bad conn.
	added := db.putConnDBLocked(dc, nil)
	db.mu.Unlock()

	if !added {
		dc.Close()
	}
}

Error log

[mysql] 2017/08/29 19:52:06 connection_go18.go:23: invalid connection

Configuration

*Driver version (or git SHA):i3UtE7/Cn57eX1hO5Z0CqY/Eeb4=

*Go version: go1.8.3 darwin/amd64

*Server version: MySQL 5.6, MariaDB 10.0.20

*Server OS: darwin/amd64

[methane]

It is intentional. When ErrBadConn is returned, database/sql will retry.
So driver must not return ErrBadConn for such cases.

[lintanghui]

@methane, but driver return err not equals to ErrBadConn. and close connection, but in standard lib ,the connection would be put back into pool, next time i would get a connection which had been close before and err invalid connection would happened.
That says, once timeout happened, next request may get a closed connection and lead to connection err.
In my opinion. if you do not return ErrBadConn. you should not close this connection because in standard lib such connection would be put back into pool.

[methane]

next time i would get a connection which had been close before and err invalid connection would happened.

Yes, and retried.

That says, once timeout happened, next request may get a closed connection and lead to connection err.

And retried transparently.

In my opinion.if you do not return ErrBadConn. you should not close this connection because in standard lib such connection would be put back into pool.

It may cause more critical result.

SELECT 1; -- if this query cancelled,
SELECT 2; -- get previous result "1"
SELECT 3; -- get previous result "2"

[lintanghui]

close connection or not, put back or not. driver should do the same behavior with standard Lib.
may be standard lib should not ignored timeout err and not put back into poll.
may be this code

added := db.putConnDBLocked(dc, nil)

should be change to below in standard lib.

added := db.putConnDBLocked(dc, err)

[methane]

I can't get what your said. "Return ErrBadConn next time" is best way for now.
This is design issue of database/sql.

[methane]

see also: golang/go#11978 (comment)

[fxm5547]

defaultDB.SetConnMaxLifetime(3595 * time.Second)

may resolve.