-
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
executor: avoid goroutine leak in index Lookup join #19251
Conversation
Please add a test. BTW, please also create an issue for this PR. |
f389d45
to
5ca6c4d
Compare
/run-check_dev_2 |
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
52b72c5
to
2b1a325
Compare
2b1a325
to
515ba52
Compare
/run-check_dev_2 |
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
/reward 300 |
This PR's linked issue is not picked. |
/reward 300 |
Reward success. |
/merge |
Your auto merge job has been accepted, waiting for:
|
/run-all-tests |
@fzhedu, Congratulations, you get 300 in this PR, and your total score is 300 in high-performance challenge program. DetailsTip : None Warning: cc: Mentor @SunRunAway |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-3.0 in PR #20791 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-4.0 in PR #20792 |
What problem does this PR solve?
Issue Number: close #19354
Problem Summary: the index lookup join leak goroutines if inner works build innerExec failed, because it returns immediately after opened, ignore closing.
besides, if one innerworker find
ctx.Done()
, it returns but lettask.innerResult
is null, which is later called but without judgement. This causes panic.What is changed and how it works?
Proposal: xxx
What's Changed:
move
defer innerExec.close
before return errors.judgen
task.innerResult
before using it.How it Works:
Related changes
Check List
Tests
Side effects
Release note