fix: stop one slow device from blocking the others - #1246
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1246 +/- ##
=======================================
Coverage 56.71% 56.71%
=======================================
Files 149 149
Lines 12154 12154
=======================================
Hits 6893 6893
Misses 5260 5260
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f15e35a to
6b2ad42
Compare
The worker ran each connection setup inline, so one stuck device held the only goroutine and the rest queued behind it.
There was a problem hiding this comment.
🟡 Changes recommended
The new worker behavior can spawn unbounded goroutines (removing backpressure), which can cause resource exhaustion and degrade overall service stability under load.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to prevent one slow/unreachable device’s WSMAN connection setup from blocking other devices by changing the WSMAN request worker to allow concurrent processing.
Changes:
- Updated the WSMAN worker loop to execute queued setup requests asynchronously.
File summaries
| File | Description |
|---|---|
| internal/usecase/devices/wsman/message.go | Changes worker behavior from serialized execution to concurrent request execution to avoid global blocking. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (g GoWSMANMessages) Worker() { | ||
| for { | ||
| select { | ||
| case request := <-requestQueue: | ||
| request() | ||
| time.Sleep(queueTickTime) | ||
| go request() |
| case request := <-requestQueue: | ||
| request() | ||
| time.Sleep(queueTickTime) | ||
| go request() |
There was a problem hiding this comment.
@amarnath-ac : moving to the asynchronous approach for sure would overcome the issue you observed with one request slowing down all subsequent requests. At the same time this brings in two issues to take care of:
- This approach allows multiple connections towards each device and they can execute concurrently, they may access the shared data at the same time which in this case is the Connections. This requires careful synchronization (like using mutexes) to prevent data races, did you check on that. Please check on this
- This approach can spawn a large number of go routines, did you check on that too
I would suggest to come out with a balanced approach where you create a fixed number of worker threads pool . Instead of spawning a new goroutine for every single request, you pre-spawn a fixed number of long-lived "worker" goroutines. These workers all listen to the same job queue and also ensure that workers handling requests for the same device ensure there are no race conditions
The worker ran each connection setup inline, so one stuck device held the only goroutine and the rest queued behind it.
Steps to reproduce:
Behavior:
Before:
Opening one device's details while another device was still loading (or was unreachable) left the second device stuck. Its calls sat behind the first one and either returned very late or timed out with 504. Every device shared a single goroutine, so one bad device slowed all of them.
After fix:
Each device's connection setup runs on its own goroutine. An unreachable device still fails on its own, but other devices load normally at the same time.