[Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks)
- Dominant language
- Go
- Stars
- 5k
- Forks
- 1k
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 31
Description
## Summary
This issue tracks potential bugs found through static analysis (code review, `go vet`, pattern matching, and manual verification) of the production code (non-test files) in the `cluster`, `config_center`, `protocol`, `registry`, `common`, `remoting`, and `graceful_shutdown` packages.
Findings are grouped by severity:
- **P0 (7)** — can cause process crash (unrecoverable `fatal error`), goroutine leaks, or fd exhaustion under normal operation
- **P1 (10)** — high-risk: swallowed errors, unchecked type assertions, missing cancellation
- **P2 (7)** — robustness / anti-pattern improvements
All line numbers refer to the current `develop` tip.
---
## 🔴 P0 — Must Fix
### 1. Concurrent map read/write in ZooKeeper `CacheListener` → fatal crash
- **File:** `config_center/zookeeper/listener.go:48-91`
- **Code:**
```go
// AddListener
listeners, loaded := l.keyListeners.LoadOrStore(key, map[...]struct{}{listener: {}})
if loaded {
listeners.(map[...]struct{})[listener] = struct{}{} // writes inner map
l.keyListeners.Store(key, listeners)
}
// DataChange
if listeners, ok := l.keyListeners.Load(event.Path); ok {
for listener := range listeners.(map[...]struct{}) { ... } // reads inner map
}
```
- **Problem:** `keyListeners` is a `sync.Map`, but the stored value is a plain `map`. `AddListener`/`RemoveListener` mutate the inner map directly while `DataChange` may `range` over the same map concurrently. This triggers Go's built-in concurrent-map detector → `fatal error: concurrent map read and map write`, which **cannot be recovered** and kills the process.
- **Fix:** Protect the inner map with its own `sync.RWMutex`, or replace with copy-on-write (build a new map and `Store` it atomically).
### 2. Goroutine leak in `ForkingClusterInvoker`
- **File:** `cluster/cluster/forking/cluster_invoker.go:74-99`
- **Problem:** A `queue.New(1)` (capacity 1) collects results. N fork goroutines each call `resultQ.Put(result)`. The caller does a single `resultQ.Poll(1, timeout)` then returns. The remaining `N-1` fork goroutines block forever on `Put` (queue full, never drained, never closed) → **leaks N-1 goroutines per invocation**.
- **Fix:** Use a buffered channel `chan result.Result` with cap = `len(selected)`; use `select { case ch <- r: default: }` for non-blocking send, and `context`-cancel the goroutines on exit.
### 3. Type-assertion panic in Apollo `makeNestedMap`
- **File:** `config_center/apollo/impl.go:211`
- **Code:** `current = current[part].(map[string]any)`
- **Problem:** If a config key is both a leaf and a parent (e.g. `a.b=1` and `a.b.c=2`), `current[part]` is a `string`, and the assertion panics. No `ok` check.
- **Fix:** `next, ok := current[part].(map[string]any); if !ok { next = make(map[string]any); current[part] = next }; current = next`
### 4. Type-assertion panic in Apollo `GetInternalProperty`
- **File:** `config_center/apollo/impl.go:121`
- **Code:** `return value.(string), nil`
- **Problem:** `cache.Get(key)` returns `interface{}`; a non-string cached value (e.g. a number or nested map) panics on the assertion.
- **Fix:** `s, ok := value.(string); if !ok { return "", perrors.New(...) }`
### 5. File-descriptor leak in `cache_manager.loadCache`
- **File:** `registry/servicediscovery/store/cache_manager.go:106-127`
- **Problem:** On decode failure the function `return err` **before** `cf.Close()`, leaking the open file. Only the success path closes it. Missing `defer cf.Close()`.
- **Fix:** `cf, err := os.Open(...); if err != nil { return err }; defer cf.Close()`
### 6. File-descriptor leak in `accesslog.openLogFile`
- **File:** `filter/accesslog/filter.go:321-349`
- **Problem:** On `Stat` failure / `Rename` failure / log-rotation paths, the freshly `OpenFile`'d `*os.File` is not closed → fd leak that accumulates over long-running services.
- **Fix:** Ensure `defer f.Close()` (or close on every error branch) before reassigning the package-level file handle.
### 7. Goroutine leak in `FailbackClusterInvoker.process`
- **File:** `cluster/cluster/failback/cluster_invoker.go:94-121`
- **Problem:** `for range ticker.C { ... }` has no `select { case <-ctx.Done(): ... }` exit; `ticker` is never `Stop()`-ed via `defer`. The goroutine only exits when `taskList.Dispose()` is called.
- **Fix:** Add `defer ticker.Stop()` and a `case <-ctx.Done(): return` branch.
---
## 🟡 P1 — High Risk
### 8. Subscription goroutine has no exit mechanism
- **File:** `registry/directory/directory.go:205-209`
- **Code:** `go func() { if err := dir.registry.Subscribe(url, dir); err != nil { ... } }()`
- **Problem:** No `context` bound to the goroutine lifecycle. If `Subscribe` blocks, `Destroy` cannot terminate it → goroutine leak.
- **Fix:** Pass a cancellable `ctx`; `cancel()` on `Destroy`.
### 9. Hessian encode errors silently ignored (request)
- **File:** `protocol/dubbo/hessian2/hessian_request.go:129,151,153,168`
- **Problem:** `_, _ = encoder.Encode(...)` discards errors; an encoding failure still sends a truncated frame, causing hard-to-diagnose decode errors on the peer side.
### 10. Hessian encode errors silently ignored (impl)
- **File:** `protocol/dubbo/impl/hessian.go:128` (and surrounding)
- **Problem:** Same class of issue as #9 in the legacy encoder path.
### 11. Triple handler registration failure swallowed
- **File:** `protocol/triple/server.go:546`
- **Problem:** `RegisterHandler` failure is logged but not propagated; service appears "up" while actually broken.
### 12. gRPC `ReflectResponse` error ignored
- **File:** `protocol/grpc/grpc_invoker.go:128`
- **Problem:** Error from `ReflectResponse` is ignored; malformed responses go undetected.
### 13. Unchecked type assertion (registry protocol)
- **File:** `registry/protocol/protocol.go:97`
- **Problem:** Type assertion without `ok`; wrong config type → panic.
### 14. Unchecked type assertion (registry directory)
- **File:** `registry/directory/directory.go:502`
- **Problem:** Type assertion without `ok` on a value pulled from a map.
### 15. `base_registry` retry loop has no ctx exit
- **File:** `registry/base_registry.go:330`
- **Problem:** Retry loop continues without honoring `context` cancellation → can block shutdown.
### 16. `getty` pool `time.Sleep` ignores cancellation
- **File:** `remoting/getty/pool.go:81`
- **Problem:** `time.Sleep` cannot be interrupted by `ctx.Done()`; connection acquisition may hang during shutdown.
### 17. `MethodMapper` chained type assertion panic risk
- **File:** `common/rpc_service.go:378`
- **Code:** `method.Func.Call(...)[0].Interface().(map[string]string)`
- **Problem:** If `MethodMapper` returns a nil interface, the chained assertion can panic.
---
## 💭 P2 — Robustness / Anti-pattern
### 18. Explicit `Unlock` without `defer`
- **File:** `registry/directory/directory.go:771-773`
- **Problem:** `registerLock.Lock(); ...; Unlock()` — a panic between leaves the lock permanently held.
- **Fix:** `defer dir.registerLock.Unlock()`.
### 19. `GetLocalIP` error ignored
- **File:** `common/host_util.go:47`
- **Problem:** `localIp, _ = gxnet.GetLocalIP()` — on failure `localIp` is empty, producing an invalid registry address silently.
### 20. Config validity errors ignored
- **File:** `remoting/getty/config.go:143,169`
- **Problem:** `defaultClientConfig.CheckValidity()` / `defaultServerConfig.CheckValidity()` results discarded.
### 21. JSON unmarshal error ignored
- **File:** `remoting/polaris/parser/parser.go:91`
- **Problem:** `json.Unmarshal(data, &searchVal)` error discarded.
### 22. `stopListen` error ignored
- **File:** `config_center/nacos/listener.go:242`
- **Problem:** Returned error from `stopListen` discarded.
### 23. `recover` without logging
- **File:** `graceful_shutdown/shutdown.go:425`
- **Problem:** A `recover()` swallows the panic silently, hiding the root cause.
### 24. Attachment type assertion (accesslog)
- **File:** `filter/accesslog/filter.go`
- **Problem:** Attachment values are type-asserted without `ok`; unexpected attachment types panic.
---
## Suggested Next Steps
1. **Immediate (P0):** fix #1–#7. #1 (ZK concurrent map) is the most severe — it can crash the process on any concurrent config push + data change.
2. **Enablers:** enable `errorlint`, `bodyclose`, `noctx`, `nilerr` in `.golangci.yml` (already discussed in `CODE_REVIEW.md`) to catch P1/P2 systematically.
3. **Tests:** add `-race` to `make test` to surface the goroutine-leak / concurrent-map issues in CI.
Happy to open PRs for any of these if maintainers confirm the preferred fix direction.
Contributor guide
Assessment
This issue has not been assessed yet.