From c9bab512026e2e8233213ca5aab5119bb680d11b Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 29 May 2020 12:54:05 -0700 Subject: [PATCH 1/2] mysqlctl: Fix connection leak in edge case of killConnection(). It's rare that we would hit this path, but if we do, we currently are leaking the connection because I forgot to call Close() on it. Signed-off-by: Anthony Yeh --- go/vt/mysqlctl/query.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/go/vt/mysqlctl/query.go b/go/vt/mysqlctl/query.go index 15d7ab23e2c..fcc7cb429ac 100644 --- a/go/vt/mysqlctl/query.go +++ b/go/vt/mysqlctl/query.go @@ -171,10 +171,12 @@ func (mysqld *Mysqld) killConnection(connID int64) error { // It might be because the connection pool is exhausted, // because some connections need to be killed! // Try to open a new connection without the pool. - killConn, connErr = mysqld.GetDbaConnection() + conn, connErr := mysqld.GetDbaConnection() if connErr != nil { return connErr } + defer conn.Close() + killConn = conn } _, err := killConn.ExecuteFetch(fmt.Sprintf("kill %d", connID), 10000, false) From a8fa863f5919d6eca413827e4222e71fe36bd1bf Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 29 May 2020 13:14:04 -0700 Subject: [PATCH 2/2] mysqlctl: Add Context to non-pooled connection methods. Signed-off-by: Anthony Yeh --- go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go | 8 ++++---- go/vt/mysqlctl/metadata_tables.go | 3 ++- go/vt/mysqlctl/mysql_daemon.go | 4 ++-- go/vt/mysqlctl/mysqld.go | 8 ++++---- go/vt/mysqlctl/query.go | 4 ++-- go/vt/mysqlctl/xtrabackupengine.go | 2 +- go/vt/vttablet/tabletmanager/rpc_lock_tables.go | 2 +- go/vt/vttablet/tabletmanager/rpc_query.go | 4 ++-- .../tabletmanager/vreplication/vplayer_flaky_test.go | 2 +- go/vt/vttablet/tabletserver/vstreamer/rowstreamer_test.go | 2 +- 10 files changed, 20 insertions(+), 19 deletions(-) diff --git a/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go b/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go index cdf443bbdb5..08849bc10e2 100644 --- a/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go +++ b/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go @@ -477,13 +477,13 @@ func (fmd *FakeMysqlDaemon) GetAppConnection(ctx context.Context) (*dbconnpool.P } // GetDbaConnection is part of the MysqlDaemon interface. -func (fmd *FakeMysqlDaemon) GetDbaConnection() (*dbconnpool.DBConnection, error) { - return dbconnpool.NewDBConnection(context.Background(), fmd.db.ConnParams()) +func (fmd *FakeMysqlDaemon) GetDbaConnection(ctx context.Context) (*dbconnpool.DBConnection, error) { + return dbconnpool.NewDBConnection(ctx, fmd.db.ConnParams()) } // GetAllPrivsConnection is part of the MysqlDaemon interface. -func (fmd *FakeMysqlDaemon) GetAllPrivsConnection() (*dbconnpool.DBConnection, error) { - return dbconnpool.NewDBConnection(context.Background(), fmd.db.ConnParams()) +func (fmd *FakeMysqlDaemon) GetAllPrivsConnection(ctx context.Context) (*dbconnpool.DBConnection, error) { + return dbconnpool.NewDBConnection(ctx, fmd.db.ConnParams()) } // SetSemiSyncEnabled is part of the MysqlDaemon interface. diff --git a/go/vt/mysqlctl/metadata_tables.go b/go/vt/mysqlctl/metadata_tables.go index 699bc57ee4e..62bb411ebb7 100644 --- a/go/vt/mysqlctl/metadata_tables.go +++ b/go/vt/mysqlctl/metadata_tables.go @@ -20,6 +20,7 @@ import ( "bytes" "fmt" + "golang.org/x/net/context" "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/log" @@ -70,7 +71,7 @@ func PopulateMetadataTables(mysqld MysqlDaemon, localMetadata map[string]string, log.Infof("Populating _vt.local_metadata table...") // Get a non-pooled DBA connection. - conn, err := mysqld.GetDbaConnection() + conn, err := mysqld.GetDbaConnection(context.TODO()) if err != nil { return err } diff --git a/go/vt/mysqlctl/mysql_daemon.go b/go/vt/mysqlctl/mysql_daemon.go index 94e439b8c0e..b11bf829296 100644 --- a/go/vt/mysqlctl/mysql_daemon.go +++ b/go/vt/mysqlctl/mysql_daemon.go @@ -78,9 +78,9 @@ type MysqlDaemon interface { // GetAppConnection returns a app connection to be able to talk to the database. GetAppConnection(ctx context.Context) (*dbconnpool.PooledDBConnection, error) // GetDbaConnection returns a dba connection. - GetDbaConnection() (*dbconnpool.DBConnection, error) + GetDbaConnection(ctx context.Context) (*dbconnpool.DBConnection, error) // GetAllPrivsConnection returns an allprivs connection (for user with all privileges except SUPER). - GetAllPrivsConnection() (*dbconnpool.DBConnection, error) + GetAllPrivsConnection(ctx context.Context) (*dbconnpool.DBConnection, error) // ExecuteSuperQueryList executes a list of queries, no result ExecuteSuperQueryList(ctx context.Context, queryList []string) error diff --git a/go/vt/mysqlctl/mysqld.go b/go/vt/mysqlctl/mysqld.go index 14ce1c91173..f58b6643cf4 100644 --- a/go/vt/mysqlctl/mysqld.go +++ b/go/vt/mysqlctl/mysqld.go @@ -1096,13 +1096,13 @@ func (mysqld *Mysqld) GetAppConnection(ctx context.Context) (*dbconnpool.PooledD } // GetDbaConnection creates a new DBConnection. -func (mysqld *Mysqld) GetDbaConnection() (*dbconnpool.DBConnection, error) { - return dbconnpool.NewDBConnection(context.TODO(), mysqld.dbcfgs.DbaConnector()) +func (mysqld *Mysqld) GetDbaConnection(ctx context.Context) (*dbconnpool.DBConnection, error) { + return dbconnpool.NewDBConnection(ctx, mysqld.dbcfgs.DbaConnector()) } // GetAllPrivsConnection creates a new DBConnection. -func (mysqld *Mysqld) GetAllPrivsConnection() (*dbconnpool.DBConnection, error) { - return dbconnpool.NewDBConnection(context.TODO(), mysqld.dbcfgs.AllPrivsWithDB()) +func (mysqld *Mysqld) GetAllPrivsConnection(ctx context.Context) (*dbconnpool.DBConnection, error) { + return dbconnpool.NewDBConnection(ctx, mysqld.dbcfgs.AllPrivsWithDB()) } // Close will close this instance of Mysqld. It will wait for all dba diff --git a/go/vt/mysqlctl/query.go b/go/vt/mysqlctl/query.go index fcc7cb429ac..4e1e07d8492 100644 --- a/go/vt/mysqlctl/query.go +++ b/go/vt/mysqlctl/query.go @@ -160,7 +160,7 @@ func (mysqld *Mysqld) killConnection(connID int64) error { // Get another connection with which to kill. // Use background context because the caller's context is likely expired, // which is the reason we're being asked to kill the connection. - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() if poolConn, connErr := getPoolReconnect(ctx, mysqld.dbaPool); connErr == nil { // We got a pool connection. @@ -171,7 +171,7 @@ func (mysqld *Mysqld) killConnection(connID int64) error { // It might be because the connection pool is exhausted, // because some connections need to be killed! // Try to open a new connection without the pool. - conn, connErr := mysqld.GetDbaConnection() + conn, connErr := mysqld.GetDbaConnection(ctx) if connErr != nil { return connErr } diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index a7c57c93365..bfe6448fe42 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -129,7 +129,7 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "xtrabackupUser must be specified.") } // use a mysql connection to detect flavor at runtime - conn, err := params.Mysqld.GetDbaConnection() + conn, err := params.Mysqld.GetDbaConnection(ctx) if conn != nil && err == nil { defer conn.Close() } diff --git a/go/vt/vttablet/tabletmanager/rpc_lock_tables.go b/go/vt/vttablet/tabletmanager/rpc_lock_tables.go index 05306615d7a..171ff210bda 100644 --- a/go/vt/vttablet/tabletmanager/rpc_lock_tables.go +++ b/go/vt/vttablet/tabletmanager/rpc_lock_tables.go @@ -45,7 +45,7 @@ func (agent *ActionAgent) LockTables(ctx context.Context) error { return errors.New("tables already locked on this tablet") } - conn, err := agent.MysqlDaemon.GetDbaConnection() + conn, err := agent.MysqlDaemon.GetDbaConnection(ctx) if err != nil { return err } diff --git a/go/vt/vttablet/tabletmanager/rpc_query.go b/go/vt/vttablet/tabletmanager/rpc_query.go index c09a8f43178..c96094bd6b8 100644 --- a/go/vt/vttablet/tabletmanager/rpc_query.go +++ b/go/vt/vttablet/tabletmanager/rpc_query.go @@ -27,7 +27,7 @@ import ( // ExecuteFetchAsDba will execute the given query, possibly disabling binlogs and reload schema. func (agent *ActionAgent) ExecuteFetchAsDba(ctx context.Context, query []byte, dbName string, maxrows int, disableBinlogs bool, reloadSchema bool) (*querypb.QueryResult, error) { // get a connection - conn, err := agent.MysqlDaemon.GetDbaConnection() + conn, err := agent.MysqlDaemon.GetDbaConnection(ctx) if err != nil { return nil, err } @@ -72,7 +72,7 @@ func (agent *ActionAgent) ExecuteFetchAsDba(ctx context.Context, query []byte, d // ExecuteFetchAsAllPrivs will execute the given query, possibly reloading schema. func (agent *ActionAgent) ExecuteFetchAsAllPrivs(ctx context.Context, query []byte, dbName string, maxrows int, reloadSchema bool) (*querypb.QueryResult, error) { // get a connection - conn, err := agent.MysqlDaemon.GetAllPrivsConnection() + conn, err := agent.MysqlDaemon.GetAllPrivsConnection(ctx) if err != nil { return nil, err } diff --git a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go index 85cc516c2a9..0ccc4fbd011 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go +++ b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go @@ -747,7 +747,7 @@ func TestUnicode(t *testing.T) { }} // We need a latin1 connection. - conn, err := env.Mysqld.GetDbaConnection() + conn, err := env.Mysqld.GetDbaConnection(context.Background()) if err != nil { t.Fatal(err) } diff --git a/go/vt/vttablet/tabletserver/vstreamer/rowstreamer_test.go b/go/vt/vttablet/tabletserver/vstreamer/rowstreamer_test.go index 36e7dfcbf11..3bba8f1512c 100644 --- a/go/vt/vttablet/tabletserver/vstreamer/rowstreamer_test.go +++ b/go/vt/vttablet/tabletserver/vstreamer/rowstreamer_test.go @@ -151,7 +151,7 @@ func TestStreamRowsUnicode(t *testing.T) { defer engine.Close() // We need a latin1 connection. - conn, err := env.Mysqld.GetDbaConnection() + conn, err := env.Mysqld.GetDbaConnection(context.Background()) if err != nil { t.Fatal(err) }