Skip to content

Commit 4731d40

Browse files
xgopilot[bot]xgopilotzhangzqsYangSen-qn
authored
fix: resolve data race in storagev2 cacheProvider (#173)
* fix: resolve data race in cacheResolver.FeedbackGood method Create a copy of the cache value instead of modifying it in-place to avoid race conditions when multiple goroutines access the same resolverCacheValue concurrently. This fixes the data race detected by the Go race detector in the storagev2 cacheProvider. Fixes #172 Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: zhangzqs <34616640+zhangzqs@users.noreply.github.com> * refactor: use RWMutex instead of copying arrays to fix data race Replace array copying approach with sync.RWMutex for better performance: - Add RWMutex to resolverCacheValue struct - Use RLock for concurrent read operations (Resolve, IsValid, ShouldRefresh) - Use Lock for write operations (FeedbackGood) - Zero memory allocation overhead - Better performance in high-frequency call scenarios Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: YangSen-qn <49863787+YangSen-qn@users.noreply.github.com> * fix: prevent deadlock in IsEqual and improve time consistency in Resolve 1. Fix potential AB-BA deadlock in IsEqual() method: - Lock mutexes in consistent order based on pointer address - Prevents deadlock when two goroutines compare same values in reverse order 2. Improve time consistency in Resolve() method: - Re-capture timestamp after acquiring write lock - Ensures accurate expiration checks in the cleanup phase Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: YangSen-qn <49863787+YangSen-qn@users.noreply.github.com> --------- Co-authored-by: xgopilot <noreply@goplus.org> Co-authored-by: zhangzqs <34616640+zhangzqs@users.noreply.github.com> Co-authored-by: yangsen <yang_sen_ys@163.com> Co-authored-by: YangSen-qn <49863787+YangSen-qn@users.noreply.github.com>
1 parent dce7b71 commit 4731d40

File tree

1 file changed

+37
-10
lines changed

1 file changed

+37
-10
lines changed

storagev2/resolver/resolver.go

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strconv"
1212
"sync"
1313
"time"
14+
"unsafe"
1415

1516
"github.com/qiniu/go-sdk/v7/internal/cache"
1617
"github.com/qiniu/go-sdk/v7/internal/log"
@@ -99,6 +100,7 @@ type (
99100
}
100101

101102
resolverCacheValue struct {
103+
mu sync.RWMutex
102104
IPs []resolverCacheValueIP `json:"ips"`
103105
RefreshAfter time.Time `json:"refresh_after"`
104106
ExpiredAt time.Time `json:"expired_at"`
@@ -225,24 +227,31 @@ func (resolver *cacheResolver) Resolve(ctx context.Context, host string) ([]net.
225227
rcv = cacheValue.(*resolverCacheValue)
226228
}
227229
now := time.Now()
230+
rcv.mu.RLock()
228231
ips := make([]net.IP, 0, len(rcv.IPs))
232+
expiredCount := 0
229233
for _, cacheValueIP := range rcv.IPs {
230234
if cacheValueIP.ExpiredAt.After(now) {
231235
ips = append(ips, cacheValueIP.IP)
236+
} else {
237+
expiredCount++
232238
}
233239
}
234-
if len(ips) < len(rcv.IPs) {
235-
newCacheValue := &resolverCacheValue{
236-
IPs: make([]resolverCacheValueIP, 0, len(rcv.IPs)),
237-
RefreshAfter: rcv.RefreshAfter,
238-
ExpiredAt: rcv.ExpiredAt,
239-
}
240+
rcv.mu.RUnlock()
241+
242+
// If some IPs expired, clean them up
243+
if expiredCount > 0 {
244+
rcv.mu.Lock()
245+
// Re-check with fresh timestamp after acquiring write lock
246+
nowLocked := time.Now()
247+
validIPs := make([]resolverCacheValueIP, 0, len(rcv.IPs)-expiredCount)
240248
for _, cacheValueIP := range rcv.IPs {
241-
if cacheValueIP.ExpiredAt.After(now) {
242-
newCacheValue.IPs = append(newCacheValue.IPs, cacheValueIP)
249+
if cacheValueIP.ExpiredAt.After(nowLocked) {
250+
validIPs = append(validIPs, cacheValueIP)
243251
}
244252
}
245-
resolver.cache.Set(cacheKey, newCacheValue)
253+
rcv.IPs = validIPs
254+
rcv.mu.Unlock()
246255
}
247256

248257
return ips, nil
@@ -260,6 +269,10 @@ func (resolver cacheResolver) FeedbackGood(ctx context.Context, host string, ips
260269
if status == cache.GetResultFromCache || status == cache.GetResultFromCacheAndRefreshAsync {
261270
rcv := cacheValue.(*resolverCacheValue)
262271
now := time.Now()
272+
273+
rcv.mu.Lock()
274+
defer rcv.mu.Unlock()
275+
263276
anyIPLiveLonger := false
264277
for i := range rcv.IPs {
265278
if isIPContains(ips, rcv.IPs[i].IP) {
@@ -270,7 +283,6 @@ func (resolver cacheResolver) FeedbackGood(ctx context.Context, host string, ips
270283
if anyIPLiveLonger {
271284
rcv.RefreshAfter = now.Add(resolver.cacheRefreshAfter)
272285
rcv.ExpiredAt = now.Add(resolver.cacheLifetime)
273-
resolver.cache.Set(cacheKey, rcv)
274286
}
275287
}
276288
}
@@ -279,6 +291,17 @@ func (resolver cacheResolver) FeedbackBad(context.Context, string, []net.IP) {}
279291

280292
func (left *resolverCacheValue) IsEqual(rightValue cache.CacheValue) bool {
281293
if right, ok := rightValue.(*resolverCacheValue); ok {
294+
// Avoid deadlock by locking in consistent order based on pointer address
295+
first, second := left, right
296+
if uintptr(unsafe.Pointer(left)) > uintptr(unsafe.Pointer(right)) {
297+
first, second = right, left
298+
}
299+
300+
first.mu.RLock()
301+
defer first.mu.RUnlock()
302+
second.mu.RLock()
303+
defer second.mu.RUnlock()
304+
282305
if len(left.IPs) != len(right.IPs) {
283306
return false
284307
}
@@ -293,10 +316,14 @@ func (left *resolverCacheValue) IsEqual(rightValue cache.CacheValue) bool {
293316
}
294317

295318
func (left *resolverCacheValue) IsValid() bool {
319+
left.mu.RLock()
320+
defer left.mu.RUnlock()
296321
return time.Now().Before(left.ExpiredAt)
297322
}
298323

299324
func (left *resolverCacheValue) ShouldRefresh() bool {
325+
left.mu.RLock()
326+
defer left.mu.RUnlock()
300327
return time.Now().After(left.RefreshAfter)
301328
}
302329

0 commit comments

Comments
 (0)