AlexStocks opened a new issue, #3246: URL: https://github.com/apache/dubbo-go/issues/3246
### Verification Checklist - [x] I have searched the [existing Issues](https://github.com/apache/dubbo-go/issues) and believe this is not a duplicate ### Go Version 1.24 ### Dubbo-go Version v3.3.1 ### OS Linux / All platforms ### Bug Description After a thorough code audit of the `common.URL` struct and its lifecycle management across the codebase, I identified **10 memory leak points** (1 P0, 5 P1, 4 P2). These leaks cause unbounded memory growth in long-running services, especially in environments with frequent service registration/deregistration or configuration changes. --- ## P0: `registryProtocol` singleton leaks `overrideListeners` and `serviceConfigurationListeners` **File**: `registry/protocol/protocol.go:59-70, 457-496` ```go type registryProtocol struct { overrideListeners *sync.Map // leaked serviceConfigurationListeners *sync.Map // leaked } ``` `Export()` (L218, L221) stores listeners into these maps, but `Destroy()` (L457-496) only cleans `bounds` and `registries` — **never** cleans `overrideListeners` or `serviceConfigurationListeners`. Each `overrideSubscribeListener` holds a reference to the full `originInvoker` (including URL and connection resources). Since `registryProtocol` is a package-level singleton (L50 `regProtocol`), these references are never GC'd. **Impact**: Every exported service leaks its listener + invoker + URL + connections. In microservice environments with frequent service lifecycle changes, memory grows proportionally to total services ever exported. **Fix**: Add `Range + Delete` cleanup for both maps in `Destroy()`. Also `Delete` old listeners in `reExport`. --- ## P1-1: URL `attributes` are write-only — no `DeleteAttribute` method exists **File**: `common/url.go:583-590` ```go func (c *URL) SetAttribute(key string, value any) { c.attributes[key] = value // write-only, no delete API } ``` The entire codebase has zero calls to delete attributes. Values stored include large objects: `*global.ApplicationConfig`, `map[string]*global.RegistryConfig`, `ServiceInfo`, `RPCService` implementations. The `filter/adaptivesvc/filter.go:96` writes an updater on **every RPC call**. **Fix**: Add `DeleteAttribute(key string)`. Clean attributes on invoker/exporter Destroy. --- ## P1-2: `RegistryDirectory.Destroy()` doesn't clean `cacheInvokersMap` **File**: `registry/directory/directory.go:549-573` ```go func (dir *RegistryDirectory) Destroy() { dir.DoDestroy(func() { invokers := dir.cacheInvokers dir.cacheInvokers = []protocolbase.Invoker{} for _, ivk := range invokers { ivk.Destroy() } // cacheInvokersMap (sync.Map) is NEVER cleaned! }) } ``` The `sync.Map` still holds references to all destroyed invokers + their URLs. If `RegistryDirectory` is held by the singleton `registryProtocol`, these references leak permanently. **Fix**: Add `dir.cacheInvokersMap.Range(func(k, _ any) bool { dir.cacheInvokersMap.Delete(k); return true })`. --- ## P1-3: `invokerBlackList` key mismatch prevents cleanup **File**: `protocol/base/rpc_status.go:40, 213` ```go // Store uses URL.Key() func SetInvokerUnhealthyStatus(invoker Invoker) { invokerBlackList.Store(invoker.GetURL().Key(), invoker) } // Delete uses GetCacheInvokerMapKey() — different format! func RemoveUrlKeyUnhealthyStatus(key string) { invokerBlackList.Delete(key) } ``` `SetInvokerUnhealthyStatus` uses `URL.Key()` as the map key, but `RemoveUrlKeyUnhealthyStatus` receives keys from `GetCacheInvokerMapKey()` which has a **different format**. The Delete never matches, so blacklisted invokers (holding full invoker + URL + connections) are **never removed**. **Fix**: Unify key generation. Or store only the key string, not the full invoker object. --- ## P1-4: `GetParams()` returns internal map reference without lock or copy **File**: `common/url.go:657-659` ```go func (c *URL) GetParams() url.Values { return c.params // no lock, no copy } ``` Callers hold the internal `params` reference indefinitely, preventing URL GC. Callers also modify it directly (e.g., `base_configuration_listener.go:111` does `delete(override, constant.AnyhostKey)` on the returned map), which is a concurrent mutation without any lock. **Fix**: Return a copy, or deprecate in favor of `CopyParams()`. --- ## P1-5: `configurators` slice only grows, never shrinks **File**: `registry/directory/directory.go:379` ```go dir.configurators = append(dir.configurators, extension.GetDefaultConfigurator(ret)) ``` Every override/configurator event appends a new `Configurator` (holding a `*common.URL`). Never replaced or trimmed. In long-running systems with frequent config pushes, this slice grows unbounded. **Fix**: Replace entire slice on new config (like `BaseConfigurationListener.Process` does at L88), not append. --- ## P2-1: `MergeURL` reads `anotherUrl.attributes` without lock **File**: `common/url.go:879-891` ```go for attrK, attrV := range anotherUrl.attributes { // no attributesLock! if _, ok := mergedURL.GetAttribute(attrK); !ok { mergedURL.attributes[attrK] = attrV } } ``` This is both a data race and a reference leak (shallow copy extends object lifetimes). --- ## P2-2: `GetCacheInvokerMapKey` creates a temporary URL on every call **File**: `common/url.go:418-428` ```go func (c *URL) GetCacheInvokerMapKey() string { urlNew, _ := NewURL(c.PrimitiveURL) // allocates a full URL object // just to extract timestamp } ``` Called on every `ServiceEvent.Key()` in hot paths. Not a leak, but unnecessary GC pressure. --- ## P2-3: `AddParam` vs `SetParam` semantic confusion **File**: `common/url.go:537-543`, `registry/protocol/protocol.go:400` ```go // isMatched() calls this repeatedly, accumulating values under same key providerUrl.AddParam(constant.CategoryKey, constant.ConfiguratorsCategory) ``` `url.Values.Add` appends (not overwrites). Each `isMatched` call adds another CategoryKey value, causing `params[CategoryKey]` to grow linearly. **Fix**: Use `SetParam` instead of `AddParam` in `isMatched`. --- ## P2-4: `registryProtocol.Destroy()` missing else branch in async cleanup **File**: `registry/protocol/protocol.go:457-496` ```go go func() { if configShutdown := config.GetShutDown(); configShutdown != nil { // cleanup path 1 } if shutdownConfRaw, ok := ...; ok { // cleanup path 2 } // NO else: if both conditions fail, goroutine exits without UnExport or Delete }() ``` **Fix**: Add else branch that unconditionally UnExport + Delete. --- ### Summary | # | Issue | Severity | Type | |---|-------|----------|------| | 1 | overrideListeners/serviceConfigurationListeners never cleaned | P0 | Leak | | 2 | attributes write-only, no delete API | P1 | Design | | 3 | cacheInvokersMap not cleaned on Destroy | P1 | Leak | | 4 | invokerBlackList key mismatch | P1 | Leak | | 5 | GetParams() returns internal reference | P1 | Leak + Race | | 6 | configurators only appended, never replaced | P1 | Leak | | 7 | MergeURL attributes access without lock | P2 | Race + Leak | | 8 | GetCacheInvokerMapKey allocates temp URL | P2 | GC pressure | | 9 | AddParam/SetParam confusion | P2 | Accumulation | | 10 | Destroy async cleanup missing else | P2 | Conditional leak | ### Suggested Fix Priority 1. Fix P0 first (listener maps cleanup in `registryProtocol.Destroy`) 2. Add `DeleteAttribute` API and clean attributes on Destroy 3. Unify blacklist key generation 4. Fix `GetParams()` to return copy 5. Replace `append` with full replacement for configurators -- 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]
