-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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 gcworker #10622
Conversation
PTAL @MyonKeminta @disksing |
Codecov Report
@@ Coverage Diff @@
## master #10622 +/- ##
===============================================
+ Coverage 77.6835% 77.693% +0.0094%
===============================================
Files 413 413
Lines 87505 87560 +55
===============================================
+ Hits 67977 68028 +51
- Misses 14376 14378 +2
- Partials 5152 5154 +2 |
/run-all-tests |
PTAL @MyonKeminta @disksing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I should add some failpoint test soon..
@disksing PTAL |
select { | ||
case taskCh <- task: | ||
case <-ctx.Done(): | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think need return err here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should run to line https://github.com/pingcap/tidb/pull/10622/files#diff-8a388eccc19df74aa4991690c5f508a5L192 to return error!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it :) LGTM!
What problem does this PR solve?
Fix goroutine leak:
What is changed and how it works?
In range task runner's
RunOnRange
function, it create many workers, and dispatch task to the workers through thetaskCh
.When the worker meets the first error, it call
cancel
, which makes all workers exit, but the dispatch loop is still writing to the channel... soRunOnRange
blocks on heretaskCh <- task
forever.In this commit I make the sender check
<-ctx.Done()
to handle the error caseCheck List
Tests
Our schrodinger test platform find this leak and it's easy to reproduce there.
Related changes