Copilot commented on code in PR #3010:
URL: https://github.com/apache/dubbo-go/pull/3010#discussion_r2328391166
##########
protocol/base/rpc_status.go:
##########
@@ -159,21 +159,42 @@ func endCount0(rpcStatus *RPCStatus, elapsed int64,
succeeded bool) {
atomic.AddInt32(&rpcStatus.total, 1)
atomic.AddInt64(&rpcStatus.totalElapsed, elapsed)
- if rpcStatus.maxElapsed < elapsed {
- atomic.StoreInt64(&rpcStatus.maxElapsed, elapsed)
+ 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
+ }
}
Review 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:
##########
@@ -159,21 +159,42 @@ func endCount0(rpcStatus *RPCStatus, elapsed int64,
succeeded bool) {
atomic.AddInt32(&rpcStatus.total, 1)
atomic.AddInt64(&rpcStatus.totalElapsed, elapsed)
- if rpcStatus.maxElapsed < elapsed {
- atomic.StoreInt64(&rpcStatus.maxElapsed, elapsed)
+ for {
+ oldValue := atomic.LoadInt64(&rpcStatus.maxElapsed)
+ if oldValue >= elapsed {
+ break
+ }
+ if atomic.CompareAndSwapInt64(&rpcStatus.maxElapsed, oldValue,
elapsed) {
+ break
+ }
}
Review 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:
##########
@@ -159,21 +159,42 @@ func endCount0(rpcStatus *RPCStatus, elapsed int64,
succeeded bool) {
atomic.AddInt32(&rpcStatus.total, 1)
atomic.AddInt64(&rpcStatus.totalElapsed, elapsed)
- if rpcStatus.maxElapsed < elapsed {
- atomic.StoreInt64(&rpcStatus.maxElapsed, elapsed)
+ 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
+ }
}
}
Review 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.
##########
protocol/base/rpc_status.go:
##########
@@ -159,21 +159,42 @@ func endCount0(rpcStatus *RPCStatus, elapsed int64,
succeeded bool) {
atomic.AddInt32(&rpcStatus.total, 1)
atomic.AddInt64(&rpcStatus.totalElapsed, elapsed)
- if rpcStatus.maxElapsed < elapsed {
- atomic.StoreInt64(&rpcStatus.maxElapsed, elapsed)
+ 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
+ }
}
Review 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.
--
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]
