Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store: fix routine/client leak when get a cached store #42779

Merged
merged 8 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions store/driver/tikv_driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func (d *TiKVDriver) setDefaultAndOptions(options ...Option) {

// OpenWithOptions is used by other program that use tidb as a library, to avoid modifying GlobalConfig
// unspecified options will be set to global config
func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (kv.Storage, error) {
func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (resStore kv.Storage, err error) {
mc.Lock()
defer mc.Unlock()
d.setDefaultAndOptions(options...)
Expand All @@ -154,7 +154,28 @@ func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (kv.Storage,
return nil, errors.Trace(err)
}

pdCli, err := pd.NewClient(etcdAddrs, pd.SecurityOption{
var (
pdCli pd.Client
spkv *tikv.EtcdSafePointKV
s *tikv.KVStore
)
defer func() {
if err != nil {
if s != nil {
// if store is created, it will close spkv and pdCli inside
_ = s.Close()
return
}
if spkv != nil {
_ = spkv.Close()
}
if pdCli != nil {
pdCli.Close()
}
}
}()

pdCli, err = pd.NewClient(etcdAddrs, pd.SecurityOption{
CAPath: d.security.ClusterSSLCA,
CertPath: d.security.ClusterSSLCert,
KeyPath: d.security.ClusterSSLKey,
Expand All @@ -176,6 +197,7 @@ func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (kv.Storage,
// FIXME: uuid will be a very long and ugly string, simplify it.
uuid := fmt.Sprintf("tikv-%v", pdCli.GetClusterID(context.TODO()))
if store, ok := mc.cache[uuid]; ok {
pdCli.Close()
return store, nil
}

Expand All @@ -184,7 +206,7 @@ func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (kv.Storage,
return nil, errors.Trace(err)
}

spkv, err := tikv.NewEtcdSafePointKV(etcdAddrs, tlsConfig)
spkv, err = tikv.NewEtcdSafePointKV(etcdAddrs, tlsConfig)
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -212,7 +234,7 @@ func (d TiKVDriver) OpenWithOptions(path string, options ...Option) (kv.Storage,
tikv.WithCodec(codec),
)

s, err := tikv.NewKVStore(uuid, pdClient, spkv, rpcClient)
s, err = tikv.NewKVStore(uuid, pdClient, spkv, rpcClient)
if err != nil {
return nil, errors.Trace(err)
}
Expand Down
16 changes: 16 additions & 0 deletions tests/realtikvtest/txntest/isolation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,28 @@ package txntest
import (
"testing"

"github.com/pingcap/tidb/store/driver"
"github.com/pingcap/tidb/testkit"
"github.com/pingcap/tidb/tests/realtikvtest"
"github.com/pingcap/tidb/util"
"github.com/stretchr/testify/require"
"go.opencensus.io/stats/view"
)

func TestGetCachedStore(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need real tikv to test it, so put it here

defer view.Stop()
var d driver.TiKVDriver
// when get the cached store, there should not have routine leak.
store1, err := d.Open(*realtikvtest.TiKVPath)
require.NoError(t, err)
defer func() {
require.NoError(t, store1.Close())
}()
store2, err := d.Open(*realtikvtest.TiKVPath)
require.NoError(t, err)
require.Equal(t, store1, store2)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add goleak.VerifyTestMain to TestMain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already has it in main_test.go in this package

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I supposed realtikvtest.RunTestMain(m) should have detected this leak. Maybe we can remove some IgnoreTopFunction now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're not in the list, like github.com/tikv/pd/client.(*client).handleResourceTokenDispatcher

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous, there's no test case to get the store twice, but in physical mode we will get one.

/*
These test cases come from the paper <A Critique of ANSI SQL Isolation Levels>.
The sign 'P0', 'P1'.... can be found in the paper. These cases will run under snapshot isolation.
Expand Down