From 7be7a57b98f82e3b6ab1da9cc2a57a7eca56eba1 Mon Sep 17 00:00:00 2001 From: Jason Mo Date: Mon, 9 Jan 2023 13:20:40 +0800 Subject: [PATCH 1/3] session: fix deadlock when get domain --- session/tidb.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/session/tidb.go b/session/tidb.go index fd4411a18518c..9e723a90c87d7 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -52,19 +52,21 @@ type domainMap struct { func (dm *domainMap) Get(store kv.Storage) (d *domain.Domain, err error) { dm.mu.Lock() - defer dm.mu.Unlock() if store == nil { for _, d := range dm.domains { // return available domain if any + dm.mu.Unlock() return d, nil } + dm.mu.Unlock() return nil, errors.New("can not find available domain for a nil store") } key := store.UUID() d = dm.domains[key] + dm.mu.Unlock() if d != nil { return } @@ -103,7 +105,9 @@ func (dm *domainMap) Get(store kv.Storage) (d *domain.Domain, err error) { return nil, err } + dm.mu.Lock() dm.domains[key] = d + dm.mu.Unlock() return } From 8a16112331387b8a3504acac1714695fbf27f460 Mon Sep 17 00:00:00 2001 From: Jason Mo Date: Mon, 9 Jan 2023 14:34:14 +0800 Subject: [PATCH 2/3] follow comments --- session/tidb.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/session/tidb.go b/session/tidb.go index 9e723a90c87d7..e8b39cdffb64a 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -42,11 +42,12 @@ import ( "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/sqlexec" + "github.com/pingcap/tidb/util/syncutil" "go.uber.org/zap" ) type domainMap struct { - mu sync.Mutex + mu syncutil.Mutex domains map[string]*domain.Domain } From 58339d68af51fe18891cd7990673970bbb12b199 Mon Sep 17 00:00:00 2001 From: Jason Mo Date: Mon, 9 Jan 2023 15:19:34 +0800 Subject: [PATCH 3/3] update --- domain/domain.go | 8 ++++++-- domain/domain_test.go | 6 +++--- session/tidb.go | 15 +++++---------- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/domain/domain.go b/domain/domain.go index b3ed976bf7e03..d01b900cdf444 100644 --- a/domain/domain.go +++ b/domain/domain.go @@ -898,7 +898,7 @@ func (do *Domain) Close() { const resourceIdleTimeout = 3 * time.Minute // resources in the ResourcePool will be recycled after idleTimeout // NewDomain creates a new domain. Should not create multiple domains for the same store. -func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duration, idxUsageSyncLease time.Duration, dumpFileGcLease time.Duration, factory pools.Factory, onClose func()) *Domain { +func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duration, idxUsageSyncLease time.Duration, dumpFileGcLease time.Duration, factory pools.Factory) *Domain { capacity := 200 // capacity of the sysSessionPool size do := &Domain{ store: store, @@ -909,7 +909,6 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio slowQuery: newTopNSlowQueries(30, time.Hour*24*7, 500), indexUsageSyncLease: idxUsageSyncLease, dumpFileGcChecker: &dumpFileGcChecker{gcLease: dumpFileGcLease, paths: []string{replayer.GetPlanReplayerDirName(), GetOptimizerTraceDirName()}}, - onClose: onClose, expiredTimeStamp4PC: types.NewTime(types.ZeroCoreTime, mysql.TypeTimestamp, types.DefaultFsp), mdlCheckTableInfo: &mdlCheckTableInfo{ mu: sync.Mutex{}, @@ -1082,6 +1081,11 @@ func (do *Domain) Init( return nil } +// SetOnClose used to set do.onClose func. +func (do *Domain) SetOnClose(onClose func()) { + do.onClose = onClose +} + func (do *Domain) initLogBackup(ctx context.Context, pdClient pd.Client) error { cfg := config.GetGlobalConfig() if pdClient == nil || do.etcdClient == nil { diff --git a/domain/domain_test.go b/domain/domain_test.go index c117ac2244b2e..bd9287fe730ec 100644 --- a/domain/domain_test.go +++ b/domain/domain_test.go @@ -68,7 +68,7 @@ func TestInfo(t *testing.T) { Storage: s, pdAddrs: []string{cluster.Members[0].GRPCURL()}} ddlLease := 80 * time.Millisecond - dom := NewDomain(mockStore, ddlLease, 0, 0, 0, mockFactory, nil) + dom := NewDomain(mockStore, ddlLease, 0, 0, 0, mockFactory) defer func() { dom.Close() err := s.Close() @@ -171,7 +171,7 @@ func TestStatWorkRecoverFromPanic(t *testing.T) { require.NoError(t, err) ddlLease := 80 * time.Millisecond - dom := NewDomain(store, ddlLease, 0, 0, 0, mockFactory, nil) + dom := NewDomain(store, ddlLease, 0, 0, 0, mockFactory) metrics.PanicCounter.Reset() // Since the stats lease is 0 now, so create a new ticker will panic. @@ -238,7 +238,7 @@ func TestClosestReplicaReadChecker(t *testing.T) { require.NoError(t, err) ddlLease := 80 * time.Millisecond - dom := NewDomain(store, ddlLease, 0, 0, 0, mockFactory, nil) + dom := NewDomain(store, ddlLease, 0, 0, 0, mockFactory) defer func() { dom.Close() require.Nil(t, store.Close()) diff --git a/session/tidb.go b/session/tidb.go index e8b39cdffb64a..113638958ecab 100644 --- a/session/tidb.go +++ b/session/tidb.go @@ -53,21 +53,19 @@ type domainMap struct { func (dm *domainMap) Get(store kv.Storage) (d *domain.Domain, err error) { dm.mu.Lock() + defer dm.mu.Unlock() if store == nil { for _, d := range dm.domains { // return available domain if any - dm.mu.Unlock() return d, nil } - dm.mu.Unlock() return nil, errors.New("can not find available domain for a nil store") } key := store.UUID() d = dm.domains[key] - dm.mu.Unlock() if d != nil { return } @@ -84,10 +82,7 @@ func (dm *domainMap) Get(store kv.Storage) (d *domain.Domain, err error) { zap.Stringer("index usage sync lease", idxUsageSyncLease)) factory := createSessionFunc(store) sysFactory := createSessionWithDomainFunc(store) - onClose := func() { - dm.Delete(store) - } - d = domain.NewDomain(store, ddlLease, statisticLease, idxUsageSyncLease, planReplayerGCLease, factory, onClose) + d = domain.NewDomain(store, ddlLease, statisticLease, idxUsageSyncLease, planReplayerGCLease, factory) var ddlInjector func(ddl.DDL) *schematracker.Checker if injector, ok := store.(schematracker.StorageDDLInjector); ok { @@ -105,10 +100,10 @@ func (dm *domainMap) Get(store kv.Storage) (d *domain.Domain, err error) { if err != nil { return nil, err } - - dm.mu.Lock() dm.domains[key] = d - dm.mu.Unlock() + d.SetOnClose(func() { + dm.Delete(store) + }) return }