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) #20863

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #20808 to release-3.0


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.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@MyonKeminta please accept the invitation then you can push to the cherry-pick pull requests.
https://github.com/ti-srebot/tidb/invitations

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>
@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 9, 2020
@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2020
@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

/run-sqllogic-test-2

@bb7133 bb7133 merged commit d47252a into pingcap:release-3.0 Nov 11, 2020
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/bug-fix This PR fixes a bug. type/3.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants