fix: monitorUpstreamConnectionState CPU consumption #3884
Conversation
`monitorUpstreamConnectionState()` is currently consuming the a lot of CPU. `monitorUpstreamConnectionState()` is a goroutine that listens for GRPC client connection changes using GRPC's `WaitForStateChange` method. It is continuously running and not actually waiting for state changes . I suspect there are a lot of GRPC connection state transitions from `CONNECTING` GRPC state to `TRANSIENT_FAILURE` GRPC state and then back to `CONNECTING` GRPC state which is triggering the continuously running loop. Adding a slight sleep to the for loop stopped the high consumption of CPU.
elimt
requested review from
talanknight,
moduli,
johanbrandhorst and
ajayreshc
| @@ -442,5 +442,7 @@ func monitorUpstreamConnectionState(ctx context.Context, cc *grpc.ClientConn, co | |||
| } | |||
|
|
|||
| connectionState.Store(newState) | |||
|
|
|||
| time.Sleep(10 * time.Millisecond) | |||
There was a problem hiding this comment.
Is this intended to be a long term solution or just a hotfix that we can get out quickly to users? It feels like we need to spend more time digging into the source of the issue. Why is the state changing so often? It could indicate something about the application that's worrying.
There was a problem hiding this comment.
Pushed another fix which should address the issue. Took out sleep change
| @@ -442,5 +442,7 @@ func monitorUpstreamConnectionState(ctx context.Context, cc *grpc.ClientConn, co | |||
| } | |||
|
|
|||
| connectionState.Store(newState) | |||
|
|
|||
| time.Sleep(10 * time.Millisecond) | |||
There was a problem hiding this comment.
The sleep is a bandaid that does not fix the bug IMO.
WaitForStateChange should properly wait for grpc if it is working properly.
It appears that line 436 should be waiting on the value retrieved on line 437 but state never gets set to that value.
There was a problem hiding this comment.
Updated the newState to state. That fixed the issue
elimt
force-pushed
the
elimt-fix-cpu-consumption
branch
from
aba7ade to
e51418d
Compare
There was a problem hiding this comment.
LGTM, as state never became newState even when it changed. If the zero value of state (Idle I believe) didn't match getState() then the result is a tight loop that never resolves itself, because it would always fall out in WaitForStateChange() without polling the channel for a new value.
EDIT: Ran the updated code, and verified. CPU is < 1.0% on my system.
Update the changelog details for High CPU Utilization bug fix: #3884
Update the changelog details for High CPU Utilization bug fix: #3884
Update the changelog details for High CPU Utilization bug fix: #3884
Bug found in #3881
monitorUpstreamConnectionState()is a goroutine that listens for GRPC client connection changes using GRPC'sWaitForStateChangemethod.monitorUpstreamConnectionState()is currently consuming the a lot of CPU. It seems to be continuously running and not actually waiting for state changes.Reference for GRPC Connection State Transitions: https://grpc.github.io/grpc/core/md_doc_connectivity-semantics-and-api.html
statewas never getting updated so the default state was never getting updated which was causing the loop not to wait for new state chanegsBefore Fix
After Fix