apache / apache/dubbo-go

[Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks)

Open
#3,552 3 comments 0 reactions 1 assignee Claimed by @XnLemon View on GitHub
☢️ Bug 3.3.3
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.