AlexStocks opened a new issue, #3552:
URL: https://github.com/apache/dubbo-go/issues/3552
## 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.
--
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]
