HGD-coder opened a new pull request, #3184:
URL: https://github.com/apache/dubbo-go/pull/3184
### Description
Fixes #3153
#### 1. The Issue (Race Condition)
As described in #3153, a panic (`runtime error: invalid memory address`)
occurs under high concurrency when `ReferenceConfig.Destroy()` is triggered:
1. `Destroy()` sets `bi.url = nil` to release resources.
2. Concurrent calls to `GetURL()` return `nil`.
3. Upper layers (Router, LoadBalance, Cluster, Filter) access methods on the
returned `nil` URL (e.g., `url.GetParam()`), causing the panic.
#### 2. Solution: Null Object Pattern
This PR introduces a **Null Object Pattern** to fix the panic safely:
- **Global Sentinel**: Adds a thread-safe `emptyURL` initialized via
`common.NewURLWithOptions`.
- **Safe Access**: Modifies `GetURL()` to return `emptyURL` instead of `nil`
when `bi.url` is destroyed.
#### 3. Why we chose this solution (Comparison of Alternatives)
We evaluated three potential solutions. Here is why the Null Object Pattern
is the best choice:
**Option A: Remove `bi.url = nil` in Destroy()**
- **Idea**: Keep the old URL even after destruction.
- **Problem**: This creates **"Zombie Invokers"**. Upper layers would still
see a valid Protocol/Host/Port and continue routing traffic to a closed client,
causing `use of closed network connection` errors. It also prevents the URL
(and its large Map) from being GC'd, causing potential **memory leaks**.
**Option B: Add `if url != nil` checks at every call site**
- **Idea**: Modify all callers (Router, Cluster, LoadBalance, etc.) to check
for nil.
- **Problem (Unfeasible)**:
- **"Whac-A-Mole" Risk**: `GetURL()` is a fundamental method used in
**50+ files**. Missing just one spot means the panic persists.
- **Maintenance Nightmare**: It forces every future developer to
remember "Check for nil after GetURL", increasing cognitive load.
**Option C: Null Object Pattern (This PR)**
- **Result**: `GetURL()` never returns nil.
- **Benefit**: Callers don't need to change. The `emptyURL` returns empty
parameters, so upper layers (like Router) will naturally filter out this
destroyed invoker (e.g., matching params fails gracefully).
- **Cost**: Negligible memory (**~429 Bytes** singleton, verified by
benchmark).
#### 4. Verification
| Approach | Bug #3153 Reproduction | Result |
| :--- | :--- | :--- |
| **Null Object (This PR)** | **Passed** (No Panic) | ✅ **Safe & Clean**.
Logic remains correct (invoker is treated as empty/unavailable). |
| Remove `url=nil` | Passed (No Panic) | ❌ **Unsafe**. Risk of traffic
routing to closed connections (Zombies). |
#### 5. Request for Review
I have verified that this change passes the reproduction script for #3153.
**Could reviewers please help check if introducing this `emptyURL` (Null
Object) might have any unexpected side effects on other modules (e.g., Service
Discovery or Metrics)?**
### Checklist
- [x] I confirm the target branch is `develop`
- [x] Code has passed local testing
- [x] I have added tests that prove my fix is effective or that my feature
works
--
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]
