You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "AlexStocks (via GitHub)" <gi...@apache.org> on 2026/07/28 02:13:34 UTC
[I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
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: notifications-unsubscribe@dubbo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
Posted by "Alanxtl (via GitHub) via notifications" <no...@dubbo.apache.org>.
Alanxtl commented on issue #3552:
URL: https://github.com/apache/dubbo-go/issues/3552#issuecomment-6544007364
done
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on issue #3552:
URL: https://github.com/apache/dubbo-go/issues/3552#issuecomment-5100943927
> pls assign me to sweep these
done
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
Posted by "Alanxtl (via GitHub) via notifications" <no...@dubbo.apache.org>.
Alanxtl closed issue #3552: [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks)
URL: https://github.com/apache/dubbo-go/issues/3552
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
Posted by "XnLemon (via GitHub)" <gi...@apache.org>.
XnLemon commented on issue #3552:
URL: https://github.com/apache/dubbo-go/issues/3552#issuecomment-5099769215
pls assign me to sweep these
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [I] [Bug] Potential bugs found via static analysis (goroutine leaks, concurrent-map crash, type-assertion panics, fd leaks) [dubbo-go]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on issue #3552:
URL: https://github.com/apache/dubbo-go/issues/3552#issuecomment-5099235078
## go vet verification (2026-07-28)
Ran `GOTOOLCHAIN=local go vet ./...` with local Go 1.26.2 on current develop tip (HEAD 53d81d17).
**Result: EXIT=0, zero warnings.**
### Coverage note
The default `go vet` analyzers (printf, structtag, copylocks, lostcancel, unreachable, unsafeptr, etc.) did **not** flag any issue in this thread. This is expected — `go vet` does not cover:
- file-descriptor leaks (needs the `bodyclose` linter)
- goroutine / context-cancellation leaks (needs `noctx` + manual review)
- unsafe type assertions without comma-ok (only the `assert` analyzer, and not these cases)
- silently ignored errors (needs `errorlint` / `nilerr`)
These are exactly the linters proposed in the enhanced golangci config (`docs/golangci-enhanced.example.yml`). Enabling them would surface these issues at PR time and prevent regressions.
### Note on the earlier run
The first `go vet ./...` used `GOTOOLCHAIN=go1.25.0+auto` and hung downloading the 1.25.0 toolchain (network). Switching to the locally-installed `go1.26.2` (`GOTOOLCHAIN=local`) completed in seconds via cached build artifacts.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org