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/06/25 06:35:56 UTC
[I] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
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: 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] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
Posted by "HarshMehta112 (via GitHub)" <gi...@apache.org>.
HarshMehta112 commented on issue #3453:
URL: https://github.com/apache/dubbo-go/issues/3453#issuecomment-4912398262
Hi @Alanxtl @AlexStocks
Closing six PRs as **"not planned"** without any explanation is disrespectful to contributors' time and effort. I spent considerable time preparing these PRs, yet they were closed without a single comment. Could you explain the reason? If these changes were never going to be accepted, why wasn't that made clear before contributors invested their time?
--
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] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
Posted by "Alanxtl (via GitHub)" <gi...@apache.org>.
Alanxtl closed issue #3453: Code audit: memory leaks, goroutine leaks, deadlocks and performance issues
URL: https://github.com/apache/dubbo-go/issues/3453
--
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] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on issue #3453:
URL: https://github.com/apache/dubbo-go/issues/3453#issuecomment-4797016697
I think the current issue body is now reasonably narrowed, but for follow-up efficiency I’d suggest handling the remaining items in two different buckets instead of treating them all the same.
### Bucket A: can be picked up directly as engineering fixes
These look concrete enough that someone can work on them without first designing a brand new repro framework:
1. **Getty config panic path**
- Scope: replace panic-style config handling with returned errors in `remoting/getty/getty_server.go`
- Expected output: small behavioral fix + focused tests for malformed config / validation failure
2. **Forking cluster losing-branch cancellation**
- Scope: make sibling requests cancelable once the first accepted result is chosen in `cluster/cluster/forking/cluster_invoker.go`
- Expected output: context/lifecycle improvement + test proving losing branches are stopped or bounded
3. **TPS limiter cache cleanup**
- Scope: define lifecycle or eviction policy for `filter/tps/limiter/method_service.go`
- Expected output: cleanup strategy + tests covering stale entry removal and no regression for normal limiter behavior
4. **panic/fatal policy audit (small-scope first)**
- Suggested approach: do **not** try to clean all panic/fatal sites in one PR
- Better split:
- `dubbo.go` / startup lifecycle
- `registry/servicediscovery/...`
- `loader.go`
- `filter/sentinel/filter.go`
### Bucket B: should ideally come with a reproducer / benchmark first
These still look valid, but a stronger proof artifact will make review much easier:
1. **RoundRobin concurrency skew**
- Best next step: add a concurrency stress test / benchmark showing distribution drift under parallel selection
- Reason: reviewers will want more than a source-level argument before accepting a synchronization redesign
2. **Graceful termination hardening**
- Best next step: add tests for timeout / result publication / cleanup ordering
- Reason: shutdown code is easy to destabilize without a clear safety net
3. **JS runtime pooled-state pollution**
- Best next step: create a focused script-router test proving global state survives across runs
- Reason: if we can reproduce contamination with a minimal script example, the fix discussion becomes much easier
### Suggested PR slicing
If different community members pick this up, I’d suggest keeping PRs narrowly scoped:
- PR 1: Getty config error propagation
- PR 2: Forking mode cancellation semantics
- PR 3: TPS limiter eviction/cleanup
- PR 4: RoundRobin repro + fix
- PR 5: graceful termination hardening
- PR 6: JS runtime isolation
- PR 7+: panic/fatal cleanup by subsystem
That slicing should make review easier than one large “audit cleanup” PR.
If helpful later, this issue can also be converted into a lightweight tracking issue with child issues per item once owners are identified.
--
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] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on issue #3453:
URL: https://github.com/apache/dubbo-go/issues/3453#issuecomment-4797157524
assign me, pls
--
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] Code audit: memory leaks, goroutine leaks, deadlocks and performance issues [dubbo-go]
Posted by "HarshMehta112 (via GitHub)" <gi...@apache.org>.
HarshMehta112 commented on issue #3453:
URL: https://github.com/apache/dubbo-go/issues/3453#issuecomment-4900661527
Hi @Alanxtl, I’ve split the changes into six small PRs, as you suggested. When you have a chance, please take a look. Thanks!
--
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