*: fix many leaks of the test case #26909

tiancaiamao · 2021-08-05T02:43:36Z

What problem does this PR solve?

Problem Summary:

When I tried to figure out the DATA RACE in #26241, I found those problems.

The PR #26241 does not introduce the DATA RACE itself, but as it reuse statement context, if some goroutine still reference the old statement context, then the DATA RACE happen.
It help us find those leaks, typically such case:

_, err : = tk.Exec("select xx ...")
// the record set is not closed and ignored... leak!
// so the background executor goroutine is still using the statement context

tk.MustExec("another sql ...")  
// DATA RACE, the reuse of the statement context conflicts with the previous executor goroutine

What is changed and how it works?

What's Changed:

Fix some leak places.

How it Works:

Release resource to avoid leak

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Most of the change is the test code.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-08-05T02:43:37Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xhebox
xiongjiwei

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

executor/adapter.go

tiancaiamao · 2021-08-05T02:48:54Z

executor/aggregate.go

@@ -298,14 +300,17 @@ func (e *HashAggExec) Close() error {

 // Open implements the Executor Open interface.
 func (e *HashAggExec) Open(ctx context.Context) error {
-	if err := e.baseExecutor.Open(ctx); err != nil {
-		return err
-	}
 	failpoint.Inject("mockHashAggExecBaseExecutorOpenReturnedError", func(val failpoint.Value) {


The failpoint is moved before the Open, because after calling Open and then error, the children Executor is not closed.

tiancaiamao · 2021-08-05T02:49:55Z

executor/aggregate.go

+		return err
+	}
+	// If panic in Open, the children executor should be closed because they are open.
+	defer closeBaseExecutor(&e.baseExecutor)


For the panic in Open case, we do not handle the resource release well.

Actually here is not just a problem in the test code, it's also a bug of the product.

set @@tidb_memtracker_size = xxx; insert into select * from t1 where a in select b from t2 ... // If Out of memory, panic in Open, then there will be goroutine leak

executor/executor_pkg_test.go

tiancaiamao · 2021-08-05T02:59:00Z

executor/explain.go

@@ -44,6 +44,10 @@ func (e *ExplainExec) Open(ctx context.Context) error {
 // Close implements the Executor Close interface.
 func (e *ExplainExec) Close() error {
 	e.rows = nil
+	if e.analyzeExec != nil && !e.executed {
+		// Open(), but Next() is not called.
+		return e.analyzeExec.Close()


Some test code run the following logic:

rs, err := Execute("explan analyze xxxxx") // Next() is not called rs.Close()

And the analyzeExec is not released ...
Fix for it.

tiancaiamao · 2021-08-05T03:00:41Z

executor/seqtest/prepared_test.go

@@ -57,16 +57,22 @@ func (s *seqTestSuite) TestPrepared(c *C) {
 		tk.MustExec("create table prepare_test (id int PRIMARY KEY AUTO_INCREMENT, c1 int, c2 int, c3 int default 1)")
 		tk.MustExec("insert prepare_test (c1) values (1),(2),(NULL)")

-		tk.MustExec(`prepare stmt_test_1 from 'select id from prepare_test where id > ?'; set @a = 1; execute stmt_test_1 using @a;`)
+		tk.MustExec(`prepare stmt_test_1 from 'select id from prepare_test where id > ?';`)


Multiple statement is ban in the real code.
Some test code are still using them ... and the resulted record set seems not to be closed correctly.

tiancaiamao · 2021-08-05T03:05:42Z

session/bootstrap.go

@@ -1378,9 +1378,6 @@ func upgradeToVer67(s Session, ver int64) {
 	if err != nil {
 		logutil.BgLogger().Fatal("upgradeToVer67 error", zap.Error(err))
 	}
-	if rs != nil {
-		defer terror.Call(rs.Close)


The defer close the result too late, do it explicitly.

func() { t1, err := Execute("sql1 ...") defer t1.Close() t2, err := Execute("sql2" ...) // t1 is not closed, so statement context is still in use // and we will have trouble reuse the statement context. }

xhebox

I remembered that recordset.Close() has an error returned, do we assert it? I will give lgtm, if you don't want to do it now.

executor/aggregate.go

executor/executor_pkg_test.go

planner/core/integration_test.go

session/bootstrap_test.go

tiancaiamao · 2021-08-05T13:35:30Z

I remembered that recordset.Close() has an error returned, do we assert it? I will give lgtm, if you don't want to do it now.

In the test case many places ignore the error. golint does not check the ignored error in the test code currently.

executor/aggregate.go

tiancaiamao · 2021-08-06T05:47:17Z

/merge

ti-chi-bot · 2021-08-06T05:47:21Z

This pull request has been accepted and is ready to merge.

Commit hash: 947f271

ti-chi-bot · 2021-08-06T05:47:29Z

@tiancaiamao: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

*: fix many leaks of the test case

26fc31d

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2021

tiancaiamao commented Aug 5, 2021

View reviewed changes

executor/adapter.go Show resolved Hide resolved

tiancaiamao commented Aug 5, 2021

View reviewed changes

github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Aug 5, 2021

tiancaiamao commented Aug 5, 2021

View reviewed changes

executor/executor_pkg_test.go Show resolved Hide resolved

tiancaiamao commented Aug 5, 2021

View reviewed changes

tiancaiamao mentioned this pull request Aug 5, 2021

*: reduce ResetContextOfStmt() object allocation #26241

Merged

11 tasks

xhebox reviewed Aug 5, 2021

View reviewed changes

tangenta reviewed Aug 5, 2021

View reviewed changes

executor/aggregate.go Outdated Show resolved Hide resolved

djshow832 reviewed Aug 5, 2021

View reviewed changes

executor/executor_pkg_test.go Show resolved Hide resolved

planner/core/integration_test.go Show resolved Hide resolved

session/bootstrap_test.go Outdated Show resolved Hide resolved

xiongjiwei reviewed Aug 6, 2021

View reviewed changes

executor/aggregate.go Outdated Show resolved Hide resolved

address comment

947f271

xiongjiwei approved these changes Aug 6, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 6, 2021

xhebox approved these changes Aug 6, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 6, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 6, 2021

Merge branch 'master' into fix-leak

ca43d0b

ti-chi-bot merged commit a8adc4c into pingcap:master Aug 6, 2021

tiancaiamao deleted the fix-leak branch August 6, 2021 05:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix many leaks of the test case #26909

*: fix many leaks of the test case #26909

tiancaiamao commented Aug 5, 2021

ti-chi-bot commented Aug 5, 2021 •

edited

Loading

tiancaiamao Aug 5, 2021

tiancaiamao Aug 5, 2021

tiancaiamao Aug 5, 2021

tiancaiamao Aug 5, 2021

tiancaiamao Aug 5, 2021

tiancaiamao Aug 5, 2021

xhebox left a comment

tiancaiamao commented Aug 5, 2021

tiancaiamao commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

*: fix many leaks of the test case #26909

*: fix many leaks of the test case #26909

Conversation

tiancaiamao commented Aug 5, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Aug 5, 2021 • edited Loading

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

tiancaiamao Aug 5, 2021

Choose a reason for hiding this comment

xhebox left a comment

Choose a reason for hiding this comment

tiancaiamao commented Aug 5, 2021

tiancaiamao commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

ti-chi-bot commented Aug 6, 2021

ti-chi-bot commented Aug 5, 2021 •

edited

Loading