Copilot commented on code in PR #1180:
URL:
https://github.com/apache/incubator-seata-go/pull/1180#discussion_r4028716273
##########
pkg/rm/tcc/fence/handler/tcc_fence_wrapper_handler.go:
##########
@@ -72,6 +80,11 @@ var (
fenceHandler *tccFenceWrapperHandler
fenceOnce sync.Once
cleanIntervalNanos atomic.Int64
+
+ fenceLogCleanRetryExhaustedTotal =
promauto.NewCounter(prometheus.CounterOpts{
+ Name: "tcc_fence_log_clean_retry_exhausted_total",
+ Help: "Number of TCC fence log identities removed from the
in-memory retry cache after cleanup retries were exhausted; database rows
remain eligible for a later scan.",
+ })
Review Comment:
Using promauto.NewCounter registers the metric on the global Prometheus
default registry as a package side-effect at init time. In library code this
can cause unexpected registration/panic on duplicate metric names in host
applications (especially if multiple versions or multiple registries are used).
Consider using prometheus.NewCounter and registering via an explicit
init/registration function (or allowing a registry to be injected), or at
minimum document the side-effect and naming expectations.
##########
pkg/rm/tcc/fence/handler/tcc_fence_wrapper_handler.go:
##########
@@ -287,65 +336,130 @@ func (handler *tccFenceWrapperHandler)
DestroyLogCleanChannel() {
func (handler *tccFenceWrapperHandler) deleteBatchFence(tx *sql.Tx, batch
[]model.FenceLogIdentity) error {
err := handler.tccFenceDao.DeleteMultipleTCCFenceLogIdentity(tx, batch)
if err != nil {
- return fmt.Errorf("delete batch fence log failed, batch: %v,
err: %v", batch, err)
+ return fmt.Errorf("delete batch fence log failed, batch: %v:
%w", batch, err)
Review Comment:
Logging/returning errors with the full `batch: %v` can produce very large
log lines and potentially leak sensitive identifiers (Xid), especially when
drainCacheOnce batches are large. Prefer reporting batch_size plus a small,
bounded sample (e.g., first identity) or a correlation id, and keep the full
batch out of logs/errors.
##########
pkg/rm/tcc/fence/handler/tcc_fence_wrapper_handler.go:
##########
@@ -204,18 +223,18 @@ func (handler *tccFenceWrapperHandler)
InitLogCleanChannel(dsn string) {
}
handler.logQueueOnce.Do(func() {
- handler.backgroundWg.Add(1)
+ handler.cleanerWg.Add(1)
go func() {
- defer handler.backgroundWg.Done()
+ defer handler.cleanerWg.Done()
handler.traversalCleanChannel(db)
}()
})
handler.logTaskOnce.Do(func() {
handler.stopLogCleanTask = make(chan struct{})
- handler.backgroundWg.Add(1)
+ handler.logTaskWg.Add(1)
go func() {
- defer handler.backgroundWg.Done()
+ defer handler.logTaskWg.Done()
handler.initLogCleanTask(db)
}()
})
Review Comment:
Critical race between InitLogCleanChannel() and DestroyLogCleanChannel():
Init releases lifecycleMutex at line 215 via defer, then later executes
logQueueOnce.Do/logTaskOnce.Do (WaitGroup.Add + goroutine start) without
holding the mutex. If DestroyLogCleanChannel() concurrently calls
logTaskWg.Wait()/cleanerWg.Wait(), this can trigger 'sync: WaitGroup misuse:
Add called concurrently with Wait' and/or leak goroutines started after
destruction begins. Fix by ensuring Init’s lifecycleMutex critical section
covers all state publication and WaitGroup.Add (i.e., hold lifecycleMutex
through logQueueOnce.Do/logTaskOnce.Do and any channel/db assignments), or
introduce an init/destroy state machine that prevents WaitGroup.Add once
teardown starts.
##########
pkg/rm/tcc/fence/handler/tcc_fence_wrapper_handler_test.go:
##########
@@ -36,6 +39,71 @@ import (
"seata.apache.org/seata-go/v2/pkg/util/log"
)
+type captureTestLogger struct {
+ mu sync.Mutex
+ warnings []string
+ errors []string
+}
+
+func (logger *captureTestLogger) Debug(v ...interface{}) {}
+func (logger *captureTestLogger) Debugf(format string, v ...interface{}) {}
+func (logger *captureTestLogger) Info(v ...interface{}) {}
+func (logger *captureTestLogger) Infof(format string, v ...interface{}) {}
+func (logger *captureTestLogger) Warn(v ...interface{}) {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ logger.warnings = append(logger.warnings, fmt.Sprint(v...))
+}
+func (logger *captureTestLogger) Warnf(format string, v ...interface{}) {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ logger.warnings = append(logger.warnings, fmt.Sprintf(format, v...))
+}
+func (logger *captureTestLogger) Error(v ...interface{}) {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ logger.errors = append(logger.errors, fmt.Sprint(v...))
+}
+func (logger *captureTestLogger) Errorf(format string, v ...interface{}) {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ logger.errors = append(logger.errors, fmt.Sprintf(format, v...))
+}
+func (logger *captureTestLogger) Panic(v ...interface{}) {}
+func (logger *captureTestLogger) Panicf(format string, v ...interface{}) {}
+func (logger *captureTestLogger) Fatal(v ...interface{}) {}
+func (logger *captureTestLogger) Fatalf(format string, v ...interface{}) {}
+
+func (logger *captureTestLogger) warningText() string {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ return strings.Join(logger.warnings, "\n")
+}
+
+func (logger *captureTestLogger) errorText() string {
+ logger.mu.Lock()
+ defer logger.mu.Unlock()
+ return strings.Join(logger.errors, "\n")
+}
+
+func installCaptureTestLogger(t *testing.T) *captureTestLogger {
+ t.Helper()
+ logger := &captureTestLogger{}
+ previous := log.GetLogger()
+ log.SetLogger(logger)
+ t.Cleanup(func() {
+ log.SetLogger(previous)
+ })
+ return logger
+}
+
+func stopTestDrainTask(handler *tccFenceWrapperHandler) {
+ if handler.stopDrainCache != nil {
+ close(handler.stopDrainCache)
+ handler.drainWg.Wait()
+ }
Review Comment:
In tests, stopTestDrainTask can panic if stopDrainCache is closed elsewhere
(e.g., if a test later calls DestroyLogCleanChannel() after stopTestDrainTask,
or if multiple cleanup paths invoke it). To make the helper robust and reduce
flakiness, guard the close with a sync.Once-like mechanism, or set
handler.stopDrainCache to nil under the same lock used in production before
closing it.
##########
pkg/rm/tcc/fence/handler/tcc_fence_wrapper_handler.go:
##########
@@ -355,79 +469,58 @@ func (handler *tccFenceWrapperHandler) drainCacheTask() {
for {
select {
case <-ticker.C:
- handler.cacheMutex.Lock()
-
- if handler.logCache.Len() == 0 {
- handler.cacheMutex.Unlock()
- continue
- }
-
- handler.dbMutex.RLock()
- db := handler.db
- handler.dbMutex.RUnlock()
- if db == nil {
- handler.cacheMutex.Unlock()
- continue
- }
+ handler.drainCacheOnce()
+ case <-handler.stopDrainCache:
+ return
+ }
+ }
- var drained []fenceLogCacheEntry
- for e := handler.logCache.Front(); e != nil; {
- next := e.Next()
+}
- ent := e.Value.(*fenceLogCacheEntry)
- drained = append(drained, *ent)
- handler.logCache.Remove(e)
+func (handler *tccFenceWrapperHandler) drainCacheOnce() {
+ handler.cacheMutex.Lock()
+ if handler.logCache.Len() == 0 {
+ handler.cacheMutex.Unlock()
+ return
+ }
- e = next
- }
+ handler.dbMutex.RLock()
+ db := handler.db
+ handler.dbMutex.RUnlock()
+ if db == nil {
+ handler.cacheMutex.Unlock()
+ return
+ }
Review Comment:
drainCacheOnce() holds cacheMutex while acquiring dbMutex (line 487). This
introduces a lock-order dependency (cacheMutex -> dbMutex) that can deadlock if
any other path ever acquires dbMutex before cacheMutex (even outside this
diff). Safer pattern: snapshot db under dbMutex first, then lock cacheMutex to
drain, or release cacheMutex before taking dbMutex (and re-check conditions).
This reduces deadlock risk and makes lock ordering easier to reason about.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
