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

util: use go-deadlock to find deadlock #40288

Merged
merged 15 commits into from
Jan 5, 2023
Merged
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ bazel_test: failpoint-enable bazel_ci_prepare

bazel_coverage_test: failpoint-enable bazel_ci_prepare
bazel $(BAZEL_GLOBAL_CONFIG) coverage $(BAZEL_CMD_CONFIG) \
--build_event_json_file=bazel_1.json --@io_bazel_rules_go//go/config:cover_format=go_cover \
--build_event_json_file=bazel_1.json --@io_bazel_rules_go//go/config:cover_format=go_cover --define gotags=deadlock \
-- //... -//cmd/... -//tests/graceshutdown/... \
-//tests/globalkilltest/... -//tests/readonlytest/... -//br/pkg/task:task_test -//tests/realtikvtest/...

Expand Down
1 change: 1 addition & 0 deletions domain/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ go_library(
"//util/replayer",
"//util/servermemorylimit",
"//util/sqlexec",
"//util/syncutil",
"@com_github_burntsushi_toml//:toml",
"@com_github_ngaut_pools//:pools",
"@com_github_pingcap_errors//:errors",
Expand Down
10 changes: 5 additions & 5 deletions domain/sysvar_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ package domain
import (
"context"
"fmt"
"sync"

"github.com/pingcap/tidb/kv"
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/sessionctx/variable"
"github.com/pingcap/tidb/util/logutil"
"github.com/pingcap/tidb/util/sqlexec"
"github.com/pingcap/tidb/util/syncutil"
"go.uber.org/zap"
"golang.org/x/exp/maps"
)
Expand All @@ -36,10 +36,10 @@ import (

// sysVarCache represents the cache of system variables broken up into session and global scope.
type sysVarCache struct {
sync.RWMutex // protects global and session maps
global map[string]string
session map[string]string
rebuildLock sync.Mutex // protects concurrent rebuild
syncutil.RWMutex // protects global and session maps
global map[string]string
session map[string]string
rebuildLock syncutil.Mutex // protects concurrent rebuild
}

func (do *Domain) rebuildSysVarCacheIfNeeded() (err error) {
Expand Down
1 change: 1 addition & 0 deletions executor/autoidtest/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ go_test(
],
flaky = True,
race = "on",
shard_count = 5,
deps = [
"//autoid_service",
"//config",
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ require (
github.com/prometheus/client_model v0.3.0
github.com/prometheus/common v0.39.0
github.com/prometheus/prometheus v0.0.0-20190525122359-d20e84d0fb64
github.com/sasha-s/go-deadlock v0.0.0-20161201235124-341000892f3d
github.com/shirou/gopsutil/v3 v3.22.9
github.com/shurcooL/httpgzip v0.0.0-20190720172056-320755c1c1b0
github.com/soheilhy/cmux v0.1.5
Expand Down Expand Up @@ -203,6 +204,7 @@ require (
github.com/ngaut/sync2 v0.0.0-20141008032647-7a24ed77b2ef // indirect
github.com/oklog/ulid v1.3.1 // indirect
github.com/olekukonko/tablewriter v0.0.5 // indirect
github.com/petermattis/goid v0.0.0-20170504144140-0ded85884ba5 // indirect
github.com/pierrec/lz4 v2.6.1+incompatible // indirect
github.com/pingcap/check v0.0.0-20200212061837-5e12011dc712 // indirect
github.com/pingcap/goleveldb v0.0.0-20191226122134-f82aafb29989 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,7 @@ github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FI
github.com/pborman/getopt v0.0.0-20180729010549-6fdd0a2c7117/go.mod h1:85jBQOZwpVEaDAr341tbn15RS4fCAsIst0qp7i8ex1o=
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
github.com/peterbourgon/g2s v0.0.0-20170223122336-d4e7ad98afea/go.mod h1:1VcHEd3ro4QMoHfiNl/j7Jkln9+KQuorp0PItHMJYNg=
github.com/petermattis/goid v0.0.0-20170504144140-0ded85884ba5 h1:rUMC+oZ89Om6l9wvUNjzI0ZrKrSnXzV+opsgAohYUNc=
github.com/petermattis/goid v0.0.0-20170504144140-0ded85884ba5/go.mod h1:jvVRKCrJTQWu0XVbaOlby/2lO20uSCHEMzzplHXte1o=
github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2 h1:JhzVVoYvbOACxoUmOs6V/G4D5nPVUW73rKvXxP4XUJc=
github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2/go.mod h1:iIss55rKnNBTvrwdmkUpLnDpZoAHvWaiq5+iMmen4AE=
Expand Down Expand Up @@ -861,6 +862,7 @@ github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQD
github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
github.com/ryanuber/columnize v2.1.0+incompatible/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
github.com/samuel/go-zookeeper v0.0.0-20161028232340-1d7be4effb13/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E=
github.com/sasha-s/go-deadlock v0.0.0-20161201235124-341000892f3d h1:yVBZEAirqhDYAc7xftf/swe8eHcg63jqfwdqN8KSoR8=
github.com/sasha-s/go-deadlock v0.0.0-20161201235124-341000892f3d/go.mod h1:StQn567HiB1fF2yJ44N9au7wOhrPS3iZqiDbRupzT10=
github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0=
github.com/sclevine/agouti v3.0.0+incompatible/go.mod h1:b4WX9W9L1sfQKXeJf1mUTLZKJ48R1S7H23Ji7oFO5Bw=
Expand Down
1 change: 1 addition & 0 deletions server/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ go_test(
"//util/resourcegrouptag",
"//util/rowcodec",
"//util/sqlexec",
"//util/syncutil",
"//util/topsql",
"//util/topsql/collector",
"//util/topsql/collector/mock",
Expand Down
7 changes: 6 additions & 1 deletion server/tidb_library_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/docker/go-units"
"github.com/pingcap/tidb/session"
"github.com/pingcap/tidb/store/mockstore"
"github.com/pingcap/tidb/util/syncutil"
"github.com/stretchr/testify/require"
)

Expand All @@ -47,5 +48,9 @@ func TestMemoryLeak(t *testing.T) {
runtime.GC()
runtime.ReadMemStats(&memStat)
// before the fix, initAndCloseTiDB for 20 times will cost 900 MB memory, so we test for a quite loose upper bound.
require.Less(t, memStat.HeapInuse-oldHeapInUse, uint64(300*units.MiB))
if syncutil.EnableDeadlock {
require.Less(t, memStat.HeapInuse-oldHeapInUse, uint64(1100*units.MiB))
} else {
require.Less(t, memStat.HeapInuse-oldHeapInUse, uint64(300*units.MiB))
}
}
1 change: 1 addition & 0 deletions session/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ go_library(
"//util/sem",
"//util/sli",
"//util/sqlexec",
"//util/syncutil",
"//util/tableutil",
"//util/timeutil",
"//util/topsql",
Expand Down
3 changes: 2 additions & 1 deletion session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ import (
"github.com/pingcap/tidb/util/sem"
"github.com/pingcap/tidb/util/sli"
"github.com/pingcap/tidb/util/sqlexec"
"github.com/pingcap/tidb/util/syncutil"
"github.com/pingcap/tidb/util/tableutil"
"github.com/pingcap/tidb/util/timeutil"
"github.com/pingcap/tidb/util/topsql"
Expand Down Expand Up @@ -274,7 +275,7 @@ type session struct {
idxUsageCollector *handle.SessionIndexUsageCollector

functionUsageMu struct {
sync.RWMutex
syncutil.RWMutex
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a plan for the rest of all other sync.RWMutex usage in TiDB?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, But deadlock detection will slow the test so I need to slowly transition to syncutil.

builtinFunctionUsage telemetry.BuiltinFunctionsUsage
}
// allowed when tikv disk full happened.
Expand Down
12 changes: 12 additions & 0 deletions util/syncutil/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "syncutil",
srcs = [
"mutex_deadlock.go",
"mutex_sync.go",
],
importpath = "github.com/pingcap/tidb/util/syncutil",
visibility = ["//visibility:public"],
deps = ["@com_github_sasha_s_go_deadlock//:go-deadlock"],
)
40 changes: 40 additions & 0 deletions util/syncutil/mutex_deadlock.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2022 PingCAP, Inc.
hawkingrei marked this conversation as resolved.
Show resolved Hide resolved
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build deadlock

package syncutil

import (
"time"

deadlock "github.com/sasha-s/go-deadlock"
)

// EnableDeadlock is a flag to enable deadlock detection.
const EnableDeadlock = true

func init() {
deadlock.Opts.DeadlockTimeout = 20 * time.Second
}

// A Mutex is a mutual exclusion lock.
type Mutex struct {
deadlock.Mutex
}

// An RWMutex is a reader/writer mutual exclusion lock.
type RWMutex struct {
deadlock.RWMutex
}
32 changes: 32 additions & 0 deletions util/syncutil/mutex_sync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2022 PingCAP, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build !deadlock

package syncutil

import "sync"

// EnableDeadlock is a flag to enable deadlock detection.
const EnableDeadlock = false

// A Mutex is a mutual exclusion lock.
type Mutex struct {
sync.Mutex
}

// An RWMutex is a reader/writer mutual exclusion lock.
type RWMutex struct {
sync.RWMutex
}