util: fix datarace and leak in TestGlobalMemoryControlForAnalyze #41548

chrysan · 2023-02-17T10:45:29Z

What problem does this PR solve?

Issue Number: close #40946 & #41511

Problem Summary:

What is changed and how it works?

Check List

Tests

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

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2023-02-17T10:45:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hawkingrei

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.

hawkingrei · 2023-02-20T06:01:44Z

/retest

hawkingrei · 2023-02-23T04:27:54Z

/retest

Signed-off-by: Weizhen Wang <[email protected]>

hawkingrei · 2023-02-23T07:38:49Z


==================
WARNING: DATA RACE
Read at 0x00c0207e81c0 by goroutine 63890:
  github.com/pingcap/tidb/util/servermemorylimit.killSessIfNeeded()
      util/servermemorylimit/servermemorylimit.go:125 +0x3f8
  github.com/pingcap/tidb/util/servermemorylimit.(*Handle).Run()
      util/servermemorylimit/servermemorylimit.go:74 +0x15e
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze.func2()
      executor/analyzetest/analyze_test.go:3079 +0x39

Previous write at 0x00c0207e81c0 by goroutine 40664:
  github.com/pingcap/tidb/executor.ResetContextOfStmt()
      executor/executor.go:2050 +0x819
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2156 +0x272
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).Exec()
      testkit/testkit.go:296 +0x4db
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3094 +0x49f
  github.com/pingcap/failpoint.(*Failpoint).Enable()
      external/com_github_pingcap_failpoint/failpoint.go:54 +0x44
  github.com/pingcap/failpoint.(*Failpoints).Enable()
      external/com_github_pingcap_failpoint/failpoints.go:105 +0x276
  github.com/pingcap/failpoint.Enable()
      external/com_github_pingcap_failpoint/failpoints.go:222 +0x435
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3089 +0x436
  github.com/pingcap/failpoint.(*Failpoint).Enable()
      external/com_github_pingcap_failpoint/failpoint.go:54 +0x44
  github.com/pingcap/failpoint.(*Failpoints).Enable()
      external/com_github_pingcap_failpoint/failpoints.go:105 +0x276
  github.com/pingcap/failpoint.Enable()
      external/com_github_pingcap_failpoint/failpoints.go:222 +0x3ef
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3088 +0x3f0
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelay()
      executor/adapter.go:805 +0x404
  github.com/pingcap/tidb/executor.(*ExecStmt).Exec()
      executor/adapter.go:600 +0x148d
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3073 +0xda
  github.com/pingcap/tidb/sessionctx/variable.glob..func174()
      sessionctx/variable/sysvar.go:904 +0x6f
  github.com/pingcap/tidb/sessionctx/variable.(*SysVar).Validate()
      sessionctx/variable/variable.go:301 +0x174
  github.com/pingcap/tidb/session.(*session).SetGlobalSysVar()
      session/session.go:1508 +0xfe
  github.com/pingcap/tidb/executor.(*SetExecutor).setSysVariable()
      executor/set.go:149 +0x7f3
  github.com/pingcap/tidb/executor.(*SetExecutor).Next()
      executor/set.go:99 +0xeb7
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x375
  github.com/pingcap/tidb/executor.(*ExecStmt).next()
      executor/adapter.go:1221 +0x9d
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelayExecutor()
      executor/adapter.go:973 +0x57c
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelay()
      executor/adapter.go:805 +0x404
  github.com/pingcap/tidb/executor.(*ExecStmt).Exec()
      executor/adapter.go:600 +0x148d
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3072 +0xba
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3071 +0x9a
  github.com/pingcap/tidb/domain.(*Domain).rebuildSysVarCache()
      domain/sysvar_cache.go:146 +0x7c4
  github.com/pingcap/tidb/domain.(*Domain).LoadSysVarCacheLoop()
      domain/domain.go:1449 +0xb9
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3350 +0x7cc
  github.com/pingcap/tidb/domain.(*Domain).GetSessionCache()
      domain/sysvar_cache.go:62 +0x71
  github.com/pingcap/tidb/session.(*session).loadCommonGlobalVariablesIfNeeded()
      session/session.go:3683 +0x11e
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2148 +0x16b
  github.com/pingcap/tidb/session.(*session).ExecuteInternal()
      session/session.go:1678 +0x365
  github.com/pingcap/tidb/domain.(*Domain).LoadPrivilegeLoop()
      domain/domain.go:1393 +0x143
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3343 +0x76c
  github.com/pingcap/tidb/testkit.bootstrap()
      testkit/mockstore.go:85 +0x84
  github.com/pingcap/tidb/testkit.CreateMockStoreAndDomain()
      testkit/mockstore.go:70 +0xd0
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3068 +0x55
  testing.tRunner()
      GOROOT/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1629 +0x47

Goroutine 63890 (running) created at:
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3079 +0x331
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelay()
      executor/adapter.go:805 +0x404
  github.com/pingcap/tidb/executor.(*ExecStmt).Exec()
      executor/adapter.go:600 +0x148d
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3073 +0xda
  github.com/pingcap/tidb/sessionctx/variable.glob..func174()
      sessionctx/variable/sysvar.go:904 +0x6f
  github.com/pingcap/tidb/sessionctx/variable.(*SysVar).Validate()
      sessionctx/variable/variable.go:301 +0x174
  github.com/pingcap/tidb/session.(*session).SetGlobalSysVar()
      session/session.go:1508 +0xfe
  github.com/pingcap/tidb/executor.(*SetExecutor).setSysVariable()
      executor/set.go:149 +0x7f3
  github.com/pingcap/tidb/executor.(*SetExecutor).Next()
      executor/set.go:99 +0xeb7
  github.com/pingcap/tidb/executor.Next()
      executor/executor.go:324 +0x375
  github.com/pingcap/tidb/executor.(*ExecStmt).next()
      executor/adapter.go:1221 +0x9d
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelayExecutor()
      executor/adapter.go:973 +0x57c
  github.com/pingcap/tidb/executor.(*ExecStmt).handleNoDelay()
      executor/adapter.go:805 +0x404
  github.com/pingcap/tidb/executor.(*ExecStmt).Exec()
      executor/adapter.go:600 +0x148d
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3072 +0xba
  github.com/pingcap/tidb/session.runStmt()
      session/session.go:2372 +0x701
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2229 +0x1024
  github.com/pingcap/tidb/testkit.(*TestKit).ExecWithContext()
      testkit/testkit.go:323 +0x791
  github.com/pingcap/tidb/testkit.(*TestKit).MustExecWithContext()
      testkit/testkit.go:132 +0xb7
  github.com/pingcap/tidb/testkit.(*TestKit).MustExec()
      testkit/testkit.go:127 +0xf7
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3071 +0x9a
  github.com/pingcap/tidb/domain.(*Domain).rebuildSysVarCache()
      domain/sysvar_cache.go:146 +0x7c4
  github.com/pingcap/tidb/domain.(*Domain).LoadSysVarCacheLoop()
      domain/domain.go:1449 +0xb9
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3350 +0x7cc
  github.com/pingcap/tidb/domain.(*Domain).GetSessionCache()
      domain/sysvar_cache.go:62 +0x71
  github.com/pingcap/tidb/session.(*session).loadCommonGlobalVariablesIfNeeded()
      session/session.go:3683 +0x11e
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      session/session.go:2148 +0x16b
  github.com/pingcap/tidb/session.(*session).ExecuteInternal()
      session/session.go:1678 +0x365
  github.com/pingcap/tidb/domain.(*Domain).LoadPrivilegeLoop()
      domain/domain.go:1393 +0x143
  github.com/pingcap/tidb/session.BootstrapSession()
      session/session.go:3343 +0x76c
  github.com/pingcap/tidb/testkit.bootstrap()
      testkit/mockstore.go:85 +0x84
  github.com/pingcap/tidb/testkit.CreateMockStoreAndDomain()
      testkit/mockstore.go:70 +0xd0
  executor/analyzetest/analyzetest_test.TestGlobalMemoryControlForAnalyze()
      executor/analyzetest/analyze_test.go:3068 +0x55
  testing.tRunner()
      GOROOT/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      GOROOT/src/testing/testing.go:1629 +0x47

Goroutine 40664 (running) created at:
  testing.(*T).Run()
      GOROOT/src/testing/testing.go:1629 +0x805
  testing.runTests.func1()
      GOROOT/src/testing/testing.go:2036 +0x8d
  testing.tRunner()
      GOROOT/src/testing/testing.go:1576 +0x216
  testing.runTests()
      GOROOT/src/testing/testing.go:2034 +0x87c
  testing.(*M).Run()
      GOROOT/src/testing/testing.go:1906 +0xb44
  go.uber.org/goleak.VerifyTestMain()
      external/org_uber_go_goleak/testmain.go:53 +0x70
  executor/analyzetest/analyzetest_test.TestMain()
      executor/analyzetest/main_test.go:29 +0x318
  main.main()
      bazel-out/k8-fastbuild/bin/executor/analyzetest/analyzetest_test_/testmain.go:228 +0x7ce
==================

It is still in existed.

ti-chi-bot · 2023-04-20T18:11:21Z

PR needs rebase.

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 kubernetes/test-infra repository.

codecov · 2023-08-07T13:23:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.6604%. Comparing base (7cac20e) to head (0ba73bf).
Report is 4952 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #41548        +/-   ##
================================================
- Coverage   73.6643%   73.6604%   -0.0040%     
================================================
  Files          1126       1126                
  Lines        359482     359482                
================================================
- Hits         264810     264796        -14     
- Misses        77585      77607        +22     
+ Partials      17087      17079         -8

ti-chi-bot · 2023-12-19T13:59:19Z

@chrysan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-ddl-test	`0ba73bf`	link	true	`/test pull-integration-ddl-test`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

ti-chi-bot added do-not-merge/invalid-title do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 17, 2023

ti-chi-bot removed the do-not-merge/needs-linked-issue label Feb 23, 2023

chrysan changed the title ~~fix datarace~~ util: fix datarace Feb 23, 2023

ti-chi-bot removed the do-not-merge/invalid-title label Feb 23, 2023

chrysan changed the title ~~util: fix datarace~~ util: fix datarace in TestGlobalMemoryControlForAnalyze Feb 23, 2023

hawkingrei approved these changes Feb 23, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 23, 2023

hawkingrei self-requested a review February 23, 2023 04:30

chrysan and others added 2 commits February 23, 2023 12:32

fix datarace

f792f66

update

5e28565

Signed-off-by: Weizhen Wang <[email protected]>

hawkingrei force-pushed the fix-flaky-test branch from a2c9a6a to 5e28565 Compare February 23, 2023 04:33

chrysan added 2 commits February 23, 2023 14:46

fix leak

0c79cc8

Merge remote-tracking branch 'pingcap/master' into fix-flaky-test

0ba73bf

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 23, 2023

chrysan changed the title ~~util: fix datarace in TestGlobalMemoryControlForAnalyze~~ util: fix datarace and leak in TestGlobalMemoryControlForAnalyze Feb 23, 2023

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: fix datarace and leak in TestGlobalMemoryControlForAnalyze #41548

util: fix datarace and leak in TestGlobalMemoryControlForAnalyze #41548

chrysan commented Feb 17, 2023 •

edited

Loading

ti-chi-bot commented Feb 17, 2023 •

edited

Loading

hawkingrei commented Feb 20, 2023

hawkingrei commented Feb 23, 2023

hawkingrei commented Feb 23, 2023

ti-chi-bot commented Apr 20, 2023

codecov bot commented Aug 7, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 19, 2023

util: fix datarace and leak in TestGlobalMemoryControlForAnalyze #41548

Are you sure you want to change the base?

util: fix datarace and leak in TestGlobalMemoryControlForAnalyze #41548

Conversation

chrysan commented Feb 17, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 17, 2023 • edited Loading

hawkingrei commented Feb 20, 2023

hawkingrei commented Feb 23, 2023

hawkingrei commented Feb 23, 2023

ti-chi-bot commented Apr 20, 2023

codecov bot commented Aug 7, 2023 • edited Loading

Codecov Report

ti-chi-bot bot commented Dec 19, 2023

chrysan commented Feb 17, 2023 •

edited

Loading

ti-chi-bot commented Feb 17, 2023 •

edited

Loading

codecov bot commented Aug 7, 2023 •

edited

Loading