fix: Ensure that some rpcStatus fields are correctly updated. #3010
Conversation
…ed, and rpcStatus.failedMaxElapsed are correctly replaced through cas. Fixes: #3006 Signed-off-by: yizhenqiang <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in RPC status tracking by replacing unsafe atomic operations with proper Compare-And-Swap (CAS) loops for updating maximum elapsed time fields.
- Replaces non-atomic read-then-store operations with CAS loops to prevent race conditions
- Ensures thread-safe updates to
maxElapsed,succeededMaxElapsed, andfailedMaxElapsedfields - Maintains the same logical behavior while fixing concurrency issues
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
protocol/base/rpc_status.go
Outdated
| for { | ||
| oldValue := atomic.LoadInt64(&rpcStatus.succeededMaxElapsed) | ||
| if oldValue >= elapsed { | ||
| break | ||
| } | ||
| if atomic.CompareAndSwapInt64(&rpcStatus.succeededMaxElapsed, oldValue, elapsed) { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The CAS loop could potentially spin indefinitely under high contention. Consider adding a limit to the number of retry attempts or using a backoff strategy to prevent excessive CPU usage.
protocol/base/rpc_status.go
Outdated
| for { | ||
| oldValue := atomic.LoadInt64(&rpcStatus.maxElapsed) | ||
| if oldValue >= elapsed { | ||
| break | ||
| } | ||
| if atomic.CompareAndSwapInt64(&rpcStatus.maxElapsed, oldValue, elapsed) { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if succeeded { | ||
| if rpcStatus.succeededMaxElapsed < elapsed { | ||
| atomic.StoreInt64(&rpcStatus.succeededMaxElapsed, elapsed) | ||
| for { | ||
| oldValue := atomic.LoadInt64(&rpcStatus.succeededMaxElapsed) | ||
| if oldValue >= elapsed { | ||
| break | ||
| } | ||
| if atomic.CompareAndSwapInt64(&rpcStatus.succeededMaxElapsed, oldValue, elapsed) { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| atomic.StoreInt32(&rpcStatus.successiveRequestFailureCount, 0) | ||
| } else { | ||
| atomic.StoreInt64(&rpcStatus.lastRequestFailedTimestamp, CurrentTimeMillis()) | ||
| atomic.AddInt32(&rpcStatus.successiveRequestFailureCount, 1) | ||
| atomic.AddInt32(&rpcStatus.failed, 1) | ||
| atomic.AddInt64(&rpcStatus.failedElapsed, elapsed) | ||
| if rpcStatus.failedMaxElapsed < elapsed { | ||
| atomic.StoreInt64(&rpcStatus.failedMaxElapsed, elapsed) | ||
|
|
||
| for { | ||
| oldValue := atomic.LoadInt64(&rpcStatus.failedMaxElapsed) | ||
| if oldValue >= elapsed { | ||
| break | ||
| } | ||
| if atomic.CompareAndSwapInt64(&rpcStatus.failedMaxElapsed, oldValue, elapsed) { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The CAS loop pattern is duplicated three times with identical logic. Consider extracting this into a helper function like updateMaxElapsed(field *int64, elapsed int64) to reduce code duplication and improve maintainability.
… and rpcStatus.failedMaxElapsed are correctly replaced through cas. Fixes: #3006 Signed-off-by: yizhenqiang <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3010 +/- ##
===========================================
- Coverage 39.63% 39.61% -0.03%
===========================================
Files 457 457
Lines 38870 38876 +6
===========================================
- Hits 15408 15402 -6
- Misses 22201 22214 +13
+ Partials 1261 1260 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
maybe you can add an ut for your PR. |
I try it |
…rpcStatus.failedMaxElapsed are correctly replaced through cas. Add UT. Fixes: #3006
|
|
I'm afraid this will reduce the performance of dubbo-go RPC calls. |
I understand that 99% of requests will break here: |
4 participants
Ensure that rpcStatus.maxElapsed, rpcStatus.succeededMaxElapsed, and rpcStatus.failedMaxElapsed are correctly replaced through cas.
Fixes: #3006