sessionctx/stmtctx: avoid unlock of unlocked mutex panic on Statement… #58629
Conversation
ti-chi-bot
bot
added
release-note-none
|
Welcome @chibiegg! |
|
Hi @chibiegg. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
Hi @chibiegg. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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-sigs/prow repository. |
pkg/sessionctx/stmtctx/stmtctx.go
Outdated
| @@ -798,120 +800,137 @@ func (sc *StatementContext) AddAffectedRows(rows uint64) { | |||
| // For compatibility with MySQL, not add the affected row cause by the foreign key trigger. | |||
| return | |||
| } | |||
| sc.mu.Lock() | |||
| defer sc.mu.Unlock() | |||
| mu := &sc.mu | |||
There was a problem hiding this comment.
I can't get the point that why using this way can avoid panic, any reference for mentioning it? From my persepctive, the panic may be related to other places.
There was a problem hiding this comment.
I wasn’t fully aware of the direct cause at that point. Your observation is indeed correct.
The direct cause was the Reset() function, which replaces sc itself, and consequently replaces mu.
As mentioned in this comment, how about locking the Mutex in Reset()?
Defined2014
added
ok-to-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #58629 +/- ##
================================================
+ Coverage 73.1093% 73.5141% +0.4047%
================================================
Files 1676 1677 +1
Lines 463354 468929 +5575
================================================
+ Hits 338755 344729 +5974
+ Misses 103788 103317 -471
- Partials 20811 20883 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I’ve applied the patch suggested in the Issue comment and have been testing it in my environment. I’m not certain if this is the only fix needed, but so far, the issue hasn’t recurred. I’ll keep observing it for a while. |
…tatementContext" This reverts commit 37ebbf0.
ti-chi-bot
bot
added
size/XS
|
@Defined2014 Thank you for the suggestion. I've updated this PR to reflect the changes that I’ve already tested and confirmed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Defined2014 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/test fast_test_tiprow |
|
@chibiegg: The specified target(s) for The following commands are available to trigger optional jobs: Use In response to this:
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-sigs/prow repository. |
|
@ti-chi-bot[bot]: The specified target(s) for
Use In response to this:
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-sigs/prow repository. |
|
/retest-required |
|
@chibiegg: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
@chibiegg: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
I received a notification from ti-chi-bot that some tests have failed. I confirmed that |
Seems |
| @@ -1839,7 +1839,11 @@ func (e *memtableRetriever) setDataForProcessList(ctx sessionctx.Context) { | |||
| continue | |||
| } | |||
|
|
|||
| if pi.StmtCtx != nil && pi.RefCountOfStmtCtx != nil && !pi.RefCountOfStmtCtx.TryIncrease() { | |||
| continue | |||
There was a problem hiding this comment.
Directly using continue here might lead to the loss of some row data. It could be a better approach to perform the check within ProcessInfo.ToRow
diff --git a/pkg/util/processinfo.go b/pkg/util/processinfo.go
index 92559da329..f8a3143ed0 100644
--- a/pkg/util/processinfo.go
+++ b/pkg/util/processinfo.go
@@ -137,7 +137,10 @@ func (pi *ProcessInfo) txnStartTs(tz *time.Location) (txnStart string) {
func (pi *ProcessInfo) ToRow(tz *time.Location) []any {
bytesConsumed := int64(0)
diskConsumed := int64(0)
- if pi.StmtCtx != nil {
+ var affectedRows any
+ var cpuUsages ppcpuusage.CPUUsages
+ if pi.StmtCtx != nil && pi.RefCountOfStmtCtx != nil && !pi.RefCountOfStmtCtx.TryIncrease(){
+ affectedRows = pi.StmtCtx.AffectedRows()
if pi.MemTracker != nil {
bytesConsumed = pi.MemTracker.BytesConsumed()
}
@@ -146,11 +149,6 @@ func (pi *ProcessInfo) ToRow(tz *time.Location) []any {
}
}
- var affectedRows any
- var cpuUsages ppcpuusage.CPUUsages
- if pi.StmtCtx != nil {
- affectedRows = pi.StmtCtx.AffectedRows()
- }
if pi.SQLCPUUsage != nil {
cpuUsages = pi.SQLCPUUsage.GetCPUUsages()
}
What problem does this PR solve?
Issue Number: close #58600
Problem Summary:
What changed and how does it work?
Similar to #39368 .
In
infoschema_reader.go, I modified the code so thatRefCountOfStmtCtxis referenced in places where StatementContext was being used.Check List
Tests
Ref fatal error: sync: unlock of unlocked mutex (v8.5.0 stmtctx.go:803) #58600 (comment), no panic anymore.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.