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/tikv: Fix goroutine leak in tikv client #20808

Merged

Conversation

MyonKeminta
Copy link
Contributor

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What problem does this PR solve?

Issue Number: close #20654

Problem Summary: There's a goroutine leak found in tikv client, and it's confirmed to be caused by forgetting closing the etcd client in EtcdSafePointKV.

The bug doesn't harm TiDB, since the tikvStore longs for the whole lifetime of the TiDB process. However, it affects other projects that make use of TiKV client. For example, CDC may open and close tikv client many times.

What is changed and how it works?

This PR fixes the goroutine leak by adding a Close interface to SafePointKV, and invoke it in Close method of tikvStore.

Related changes

  • Need to cherry-pick to the release branch
    • release 4.0
    • release 3.0
      The problem should also exists in release 2.x, but I'm not sure if it's necessary. Nor do I know if we still have release plan of 2.1.x.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

The code segment of the test program:

func main() {
	go func() {
		log.Println(http.ListenAndServe("localhost:6060", nil))
	}()

	_ = store.Register("tikv", tikv.Driver{})

	uri := "tikv://xxx.xxx.xxx.xxx:2379?disableGC=true"

	for {
		var times int
		fmt.Scanf("%d", &times)

		for i := 0; i < times; i++ {
			tiStore, err := store.New(uri)
			if err != nil {
				panic(err)
			}
			err = tiStore.Close()
			if err != nil {
				panic(err)
			}
		}
	}
}

Run and type 100 (create and close tikv client 100 times), dump the goroutines by pprof after the log stopps:

goroutine profile: total 606

After the fix and run again:

goroutine profile: total 5

Side effects

-

Release note

  • Fix an goroutine leak issue in TiKV client.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta
Copy link
Contributor Author

@cfzjywxk @tiancaiamao PTAL
cc @leoppro

Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 3, 2020
@cfzjywxk
Copy link
Contributor

cfzjywxk commented Nov 3, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 3, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 3, 2020
@cfzjywxk
Copy link
Contributor

cfzjywxk commented Nov 3, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 3, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@MyonKeminta merge failed.

@MyonKeminta
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 20813
  • 20551

@MyonKeminta
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 20551

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #20863

@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 failed

@MyonKeminta MyonKeminta deleted the m/fix-tikv-client-goroutine-leak branch November 5, 2020 07:29
@MyonKeminta
Copy link
Contributor Author

/run-cherry-picker

@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20870

MyonKeminta added a commit to ti-srebot/tidb that referenced this pull request Nov 5, 2020
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
MyonKeminta added a commit to ti-srebot/tidb that referenced this pull request Nov 5, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

cherry pick pingcap#20808 to release-3.0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
ti-srebot added a commit that referenced this pull request Nov 11, 2020
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

found the goroutine leak in tikv client
4 participants