AlexStocks opened a new issue, #3248:
URL: https://github.com/apache/dubbo-go/issues/3248
## Background
After auditing the dubbo-go codebase for memory leaks (#3246) and data races
(#3247), the root causes point to several structural issues that have
accumulated over the project's 10-year history. This issue proposes a
prioritized improvement roadmap.
---
## 1. Redesign `common.URL` — the core data structure needs a major overhaul
`common.URL` is the single most important type in the framework. Every
invoker, exporter, registry, and configuration path revolves around it. But it
has grown organically into a god object with serious problems:
**Current issues:**
- `params` (url.Values) exposed via `GetParams()` without lock or copy —
callers mutate internals
- `attributes` (map[string]any) is write-only — no `DeleteAttribute`, no
`ClearAttributes`
- `AddParam` vs `SetParam` semantic confusion (`Add` appends, not
overwrites) causing silent accumulation
- `MergeURL` reads `anotherUrl.attributes` without `attributesLock` — data
race
- `GetCacheInvokerMapKey` allocates a full temporary URL on every call —
unnecessary GC pressure
- `SubURL` pointer creates implicit reference chains that prevent GC
- Multiple key generation methods (`Key()`, `GetCacheInvokerMapKey()`,
`ServiceKey()`) with different formats, causing key mismatch bugs (e.g.,
invoker blacklist)
**Proposed redesign:**
1. Make `params` immutable after construction — provide `WithParam(k, v)`
that returns a new URL (or at minimum, return copies from getters)
2. Add `DeleteAttribute(key)` and `ClearAttributes()` methods
3. Deprecate `AddParam` — it's almost always misused when `SetParam` is
intended
4. Unify key generation into a single canonical `Key()` method
5. Consider splitting URL into `URLSpec` (immutable identity) + `URLState`
(mutable runtime state)
6. Add `Clone()` that does proper deep copy of attributes
---
## 2. Make `common/extension/` concurrency-safe — introduce `Registry[T]`
The extension package has 20+ global maps, all following the same unsafe
pattern (no locks, some returning internal map references). This is the single
largest source of potential crashes.
**Proposed solution:** A generic thread-safe registry:
```go
package extension
type Registry[T any] struct {
mu sync.RWMutex
items map[string]T
}
func NewRegistry[T any]() *Registry[T] { ... }
func (r *Registry[T]) Register(name string, v T) { ... }
func (r *Registry[T]) Get(name string) (T, bool) { ... }
func (r *Registry[T]) MustGet(name string) T { ... } // panics if missing
func (r *Registry[T]) Unregister(name string) { ... }
func (r *Registry[T]) Snapshot() map[string]T { ... } // returns copy
func (r *Registry[T]) Names() []string { ... } // returns copy
```
This single abstraction would fix all 22 files in `common/extension/` and
eliminate an entire class of bugs. It also standardizes the API (current code
mixes `Set*`/`Get*`/`Unregister*`/`GetAll*` inconsistently).
---
## 3. Implement proper lifecycle management for invokers/exporters
Current state:
- `registryProtocol.Destroy()` doesn't clean `overrideListeners` or
`serviceConfigurationListeners`
- `RegistryDirectory.Destroy()` doesn't clean `cacheInvokersMap`
- Invoker blacklist entries become permanently orphaned due to key mismatch
- `exporterChangeableWrapper` URLs are set *after* being stored in the map,
creating a race window
- No `Closeable`/`Lifecycle` interface enforced — each component invents its
own cleanup pattern
**Proposed improvements:**
1. Define a `Lifecycle` interface: `Start()`, `Stop()`, `IsRunning() bool`
2. Enforce cleanup contracts: every `Store` must have a corresponding
`Delete` path
3. Add integration tests that verify zero goroutine/memory leaks after full
startup-shutdown cycle
4. Use `context.Context` consistently for cancellation (replace `time.Sleep`
polling patterns in nacos, etc.)
---
## 4. Enforce `go test -race` in CI
Currently, many data races go undetected because `-race` is not mandatory in
CI. Given the scale of race conditions found (#3247), this is the single most
impactful process change.
**Action items:**
1. Add `-race` flag to CI test runs (at minimum for core packages)
2. Fix all existing race detector failures
3. Block merges on race detector failures
---
## 5. API hygiene — stop exposing internal state
Several public APIs return internal references that callers can mutate,
breaking encapsulation:
| API | Returns | Risk |
|-----|---------|------|
| `URL.GetParams()` | Internal `url.Values` map | Caller mutation + race |
| `GetDefaultConfigReader()` | Internal `defaults` map | Concurrent
iteration crash |
| `GetRouterFactories()` | Internal `routers` map | Concurrent iteration
crash |
| `GetProviderServiceMap()` | Internal `proServices` map | Concurrent
iteration crash |
| `GetConsumerServiceMap()` | Internal `conServices` map | Concurrent
iteration crash |
| `GetAllCustomShutdownCallbacks()` | Internal `*list.List` | Concurrent
mutation |
| `GetCustomizers()` | Internal slice reference | Concurrent append+sort |
**Rule**: Public getters must return copies, never internal references. This
should be a review checklist item.
---
## 6. Reduce `config` package coupling
The ongoing refactor (PRs #3231, #3232) to remove `config` package
dependencies from protocol implementations is the right direction. But the
approach needs more care:
- **Don't silently change error semantics** (e.g., `LoadRegistries` changing
from panic to silent skip)
- **Don't break public APIs without deprecation** (e.g., `WithMethod`
signature change)
- **Validate configuration at init time** (don't remove init-time validation
without replacement)
- **Use URL attributes consistently** — document which attributes are
expected and where they're set
---
## 7. Standardize error handling — eliminate `panic` in library code
The extension package uses `panic` for missing registrations:
```go
func GetProtocol(name string) base.Protocol {
if protocols[name] == nil {
panic("protocol for [" + name + "] is not existing, ...")
}
}
```
This is inappropriate for a library/framework. A missing extension should
return an error, not crash the entire process. The caller can decide whether to
panic.
**Rule**: Reserve `panic` for truly unrecoverable programmer errors.
Extension lookups should return `(T, error)`.
---
## Priority
| Priority | Item | Impact | Effort |
|----------|------|--------|--------|
| P0 | Fix `dubbo.go:301` wrong lock | Crash prevention | 1 line |
| P0 | Enable `-race` in CI | Prevent future regressions | Config change |
| P0 | `extension.Registry[T]` | Fix 20+ race-prone files | Medium (1 week) |
| P1 | URL `DeleteAttribute` + cleanup | Memory leak prevention | Small |
| P1 | Fix `RouterChain.Route` lockless read | Crash prevention | Small |
| P1 | Return copies from public getters | Crash prevention | Small |
| P2 | URL redesign | Long-term maintainability | Large |
| P2 | Lifecycle interface | Leak prevention | Medium |
| P2 | Eliminate panics | API quality | Medium |
Happy to help with implementation of any of these items. Related issues:
#3246, #3247.
--
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]
