-
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
gc_worker: fix deadlock in physicalScanAndResolveLocks #16393
Conversation
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #16393 +/- ##
===========================================
Coverage 80.3941% 80.3941%
===========================================
Files 507 507
Lines 137561 137561
===========================================
Hits 110591 110591
Misses 18304 18304
Partials 8666 8666 |
LGTM. Is there any way we can confirm the issue is resolved? |
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
I added a test. |
@MyonKeminta PTAL |
/test |
/run-unit-test |
/run-all-tests |
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.
Rest LGTM
if err != nil { | ||
logutil.Logger(innerCtx).Error("resolve locks failed", zap.Error(err)) | ||
logutil.Logger(ctx).Error("resolve locks failed", zap.Error(err)) | ||
errCh <- err |
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 here it should check ctx too.
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's a buffered channel.
/merge |
/run-all-tests |
@youjiali1995 merge failed. |
/merge |
/run-all-tests |
cherry pick to release-4.0 in PR #16915 |
Signed-off-by: youjiali1995 zlwgx1023@gmail.com
What problem does this PR solve?
Issue Number: close #16304
Problem Summary:
It's possible to block on channel sending in physicalScanAndResolveLocks when error occours.
What is changed and how it works?
What's Changed:
Check
ctx.Done()
when sending to a channel.How it Works:
Related changes
Check List
Tests
Side effects
Release note
Fix the issue that GC worker blocks on channel sending forever when error occours.