fix(router/affinity): prevent panic caused by nil return in initialization phase#3216
fix(router/affinity): prevent panic caused by nil return in initialization phase#3216Aetherance wants to merge 1 commit intoapache:developfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3216 +/- ##
===========================================
+ Coverage 46.76% 47.91% +1.15%
===========================================
Files 295 463 +168
Lines 17172 33749 +16577
===========================================
+ Hits 8031 16172 +8141
- Misses 8287 16269 +7982
- Partials 854 1308 +454 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1f6709e to
4a1956d
Compare
| // NewRouterChain init router chain | ||
| // Loop routerFactories and call NewRouter method | ||
| func NewRouterChain(url *common.URL) (*RouterChain, error) { | ||
| if url.GetParam(constant.ApplicationKey, "") == "" { |
There was a problem hiding this comment.
这里整个过程需要加上锁吧?
假设gr1 在 line 112这里获取的值为空,运行到 line114 的时候,另外一个 gr2 在 line 112 也获取为空后进入了这 if 条件,进而导致两个 gr 竞争进行 SetParam,这就不对了吧?
There was a problem hiding this comment.
fixed.
plz review again
…ation phase Signed-off-by: Aetherance <inkToAether@gmail.com>
4a1956d to
f0f2cd8
Compare
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the affinity router initialization when the application name is not present in the URL by propagating it from the SubURL. The panic occurred because newApplicationAffinityRouter returns nil when the application name is empty, causing a nil pointer dereference during router chain operations.
Changes:
- Added logic to populate the application name from SubURL to the main URL in NewRouterChain
- Added a mutex to serialize access during the application name population
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| appNameSetMu.Lock() | ||
| if url.GetParam(constant.ApplicationKey, "") == "" { | ||
| if url.SubURL != nil { | ||
| if appName := url.SubURL.GetParam(constant.ApplicationKey, ""); appName != "" { | ||
| url.SetParam(constant.ApplicationKey, appName) | ||
| } | ||
| } | ||
| } | ||
| appNameSetMu.Unlock() |
There was a problem hiding this comment.
The mutex usage here is unnecessary and incorrect. The URL.SetParam method already has internal locking (paramsLock) for thread safety, as seen in common/url.go:558-564. Additionally, this global mutex serializes all NewRouterChain calls across the entire application, which could become a performance bottleneck when creating multiple router chains concurrently. Since URL modification is already thread-safe, the mutex should be removed.
| appNameSetMu.Lock() | ||
| if url.GetParam(constant.ApplicationKey, "") == "" { | ||
| if url.SubURL != nil { | ||
| if appName := url.SubURL.GetParam(constant.ApplicationKey, ""); appName != "" { | ||
| url.SetParam(constant.ApplicationKey, appName) | ||
| } | ||
| } | ||
| } | ||
| appNameSetMu.Unlock() |
There was a problem hiding this comment.
The PR description claims tests have been added, but there are no new test files or test functions in this change. A unit test should be added to verify that the application name is correctly propagated from SubURL when the main URL doesn't have it set, and that the fix prevents the nil panic in ApplicationAffinityRouter initialization.



Description
在一些情况下,client 配置 affinity router 时,会因为 nil 而触发段错误
该 bug 只会在配置了 application 模式的 affinity router 上出现,如果配置 service 模式的 affinity router 则不会触发该 bug
配置代码:
client:
server:
bug 原因是当前版本的client应用名未能被正确传递至 router chain。这导致 router chain 在 初始化 affinity router 时,由于获取到空应用名而直接返回 nil。最终造成 nil 被访问而 panic
本 pr 添加了在 router chain 读取应用名的逻辑来修复此 bug
Fixes #3203
Checklist
develop