AlexStocks opened a new issue, #3453:
URL: https://github.com/apache/dubbo-go/issues/3453
## Overview
A comprehensive code audit was performed on the dubbo-go codebase (932 Go
source files) focusing on memory leaks, goroutine leaks, deadlocks, performance
issues, and critical bugs.
---
## 🔴 Critical
### 1. Race condition in RoundRobin load balancer causes uneven distribution
**File**: `cluster/loadbalance/roundrobin/loadbalance.go:101-107, 160-166`
```go
// increaseCurrent() uses atomic add, but the select loop compares
// currentWeight > maxCurrentWeight where the value may already be modified
by other goroutines
func (robin *weightedRoundRobin) increaseCurrent() int64 {
return atomic.AddInt64(&robin.current, atomic.LoadInt64(&robin.weight))
}
```
**Impact**: Under high concurrency, some nodes receive far more requests
than expected. This is deterministic in production under load.
**Suggested fix**: Use CAS operations to protect the entire selection
process, or use a mutex around the select loop.
---
### 2. Getty Server panics on configuration errors, preventing service
startup
**File**: `remoting/getty/getty_server.go:102, 106, 111`
```go
if err := srvConf.CheckValidity(); err != nil {
panic(err) // Should return error instead
}
```
**Impact**: Service fails to start entirely when user configuration is
malformed. This is especially problematic during config migration/changes in
production.
**Suggested fix**: Return error instead of panic, allow caller to handle
gracefully.
---
## 🟠High
### 3. Race condition in graceful termination - shutdownResult unprotected
**File**: `graceful_shutdown/shutdown.go:217, 224, 155`
`shutdownResult` is read/written across multiple goroutines without
synchronization. Additionally, `time.AfterFunc` timeout calls `os.Exit(0)`
directly, bypassing all cleanup.
**Suggested fix**: Use `atomic.Value` or mutex protection + `sync.Once`.
---
### 4. Potential deadlock in Getty Client selectSession due to nested locks
**File**: `remoting/getty/getty_client.go:254-283`
Holds `c.mux.RLock()` while acquiring `c.gettyClientMux.Lock()`. If another
goroutine holds `gettyClientMux` and waits for `mux`, deadlock occurs.
**Trigger**: Concurrent connection creation + client closure.
**Suggested fix**: Establish consistent lock ordering or use a single lock.
---
### 5. Accesslog filter panics on closed channel
**File**: `filter/accesslog/filter.go:125-132, 409-418`
`close()` closes `logChan` while `logIntoChannel()` may still be sending,
causing "send on closed channel" panic.
**Suggested fix**: Use recover() or check context before sending.
---
### 6. Service Discovery metadata cache grows unbounded
**File**:
`registry/servicediscovery/service_instances_changed_listener_impl.go:98-157`
When providers restart frequently, `revisionToMetadata` accumulates old
revisions. `metaCache` holds references to all historical data, leading to OOM
after long runtime.
**Suggested fix**: Implement LRU/TTL eviction for metadata cache.
---
## 🟡 Medium
| # | Issue | File | Impact |
|---|-------|------|--------|
| 7 | ProviderActiveCount counter leak |
`filter/graceful_shutdown/provider_filter.go:84-97` | Counter never decrements
on filter chain error, termination times out |
| 8 | RoundRobin/ConsistentHashing global maps grow unbounded |
`cluster/loadbalance/roundrobin/loadbalance.go:40`,
`consistenthashing/loadbalance.go:42` | Memory grows continuously over time |
| 9 | `time.After()` in hot paths causes memory pressure |
`filter/accesslog/filter.go:244`, `shutdown.go:421` | Timers not GC'd until
they fire |
| 10 | TPS limiter sync.Map never cleaned |
`filter/tps/limiter/method_service.go:176` | Method+service combinations
accumulate indefinitely |
| 11 | js_instance sync.Pool Runtime state pollution |
`cluster/router/script/instance/js_instance.go:84` | goja.Runtime retains
global variables between uses |
---
## 🟢 Low
- `common/url.go`: URL.String() acquires lock frequently on hot path
- `registry/directory/directory.go`: Subscribe goroutine has no context
cancellation
- `filter/accesslog`: File handle leak in error paths
---
## False Positives (Excluded)
- p2c sync.Pool: Correct usage, rand.Rand state is intentional
- Consumer filter closingInvokers: `isClosingInvoker()` deletes expired
entries
- Triple protocol panic on serialization: Fail-fast at initialization is
acceptable
---
## Overall Assessment
**Code quality**: B+ (Good). Clear module separation, strong concurrency
awareness, comprehensive graceful termination design.
**Most risky modules**: `remoting/getty` (deadlock+panic) >
`cluster/loadbalance/roundrobin` (race) > `registry/servicediscovery` (memory
leak)
**Suggested fix roadmap**:
- **Phase 1** (1-2 weeks): Fix Critical #1-2 + Getty deadlock
- **Phase 2** (2-4 weeks): Fix termination race + cache cleanup + accesslog
panic
- **Phase 3** (1-2 months): Unify lock hierarchy + add race/deadlock static
analysis
--
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]
