Fix/3246 url memory leaks

[nagisa-kunhah]

Description

Fixes #3246

Checklist

• I confirm the target branch is develop
• Code has passed local testing
• I have added tests that prove my fix is effective or that my feature works

[Alanxtl]

our project use import-formatter to format import blocks, that's the reason why ur CI fails. For you, u should

1. run go install github.com/dubbogo/tools/cmd/imports-formatter@latest
2. cd to the root dir of dubbo-go
3. run imports-formatter

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.39326% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.55%. Comparing base (60d1c2a) to head (c3b5c45).
⚠️ Report is 767 commits behind head on develop.

Files with missing lines	Patch %	Lines
registry/mock_registry.go	0.00%	9 Missing ⚠️
common/url.go	92.59%	1 Missing and 1 partial ⚠️
registry/protocol/protocol.go	88.23%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3282      +/-   ##
===========================================
+ Coverage    46.76%   49.55%   +2.78%     
===========================================
  Files          295      469     +174     
  Lines        17172    34670   +17498     
===========================================
+ Hits          8031    17181    +9150     
- Misses        8287    16108    +7821     
- Partials       854     1381     +527     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nagisa-kunhah]

here are some confusement about this issue

1. For P1–5, my understanding is that the original behavior may have been to append the new config and apply it together with the existing one. However, based on BaseConfigurationListener.Process, it seems the behavior there is to replace the existing config entirely. Which behavior is actually expected here?
2. For P2-2, the original behavior in isMatched is to append. Replacing it with SetParam may change that behavior. Is that expected?
   @Alanxtl @AlexStocks

[Alanxtl]

In refreshAllInvokers, a batch containing only configurator URLs now returns early after replaceConfigurators(...). This updates dir.configurators, but seems to skip rebuilding/reapplying overrides to the existing cached invokers.
Could this delay configurator-only updates until the next provider event? It may be worth adding a test that starts with cached provider invokers, sends a configurator-only NotifyAll, and verifies the effective invoker URLs are refreshed immediately.

[nagisa-kunhah]

I think there are two possible behaviors for configurator-only NotifyAll, and I’d like to confirm which one we want before changing the rebuild logic.

Example:

• original provider URL:
  dubbo://10.0.0.8:20880/com.foo.UserService?group=g1&version=1.0.0&serialization=fastjson&warmup=100
• consumer reference URL:
  dubbo://127.0.0.1/com.foo.UserService?group=g1&version=1.0.0&timeout=3000&cluster=failover

Then suppose we receive one configurator-only batch:

• first batch:
  override://0.0.0.0/com.foo.UserService?timeout=2s&serialization=hessian2

and later another configurator-only batch:

• next batch:
  override://0.0.0.0/com.foo.UserService?cluster=mock3

If we rebuild from the original provider URL each time, the final effective URL would become:

dubbo://10.0.0.8:20880/com.foo.UserService?group=g1&version=1.0.0&serialization=fastjson&warmup=100&timeout=3000&cluster=mock3

If we instead re-apply the new configurator batch on top of the current effective invoker URL, the final URL would become:

dubbo://10.0.0.8:20880/com.foo.UserService?group=g1&version=1.0.0&serialization=hessian2&warmup=100&timeout=2s&cluster=mock3

Since this patch changed configurator handling from append to replace-by-batch, I’m leaning toward the first behavior, but I’d like to confirm the expected semantics first.

[Alanxtl]

here are some confusement about this issue

1. For P1–5, my understanding is that the original behavior may have been to append the new config and apply it together with the existing one. However, based on BaseConfigurationListener.Process, it seems the behavior there is to replace the existing config entirely. Which behavior is actually expected here?
2. For P2-2, the original behavior in isMatched is to append. Replacing it with SetParam may change that behavior. Is that expected?
   @Alanxtl @AlexStocks

as for 2, do you mean P2-3?
If the goal is only to avoid duplicate growth while preserving append semantics, another option would be to append only when configurators is not already present.

[AlexStocks]

Code Review: fix/3246 url memory leaks

评价

内存泄漏修复方案合理，通过引入 primitiveTS 字段避免重复解析 URL，并通过 CopyParams() 返回深拷贝解决并发访问问题。

[P1] 规范级问题

1. common/url.go:307-320 - deprecated 方法设计问题

GetParams() 标记为 deprecated 但实现已改为返回深拷贝，这可能导致：

• 调用方误以为返回的是原始 map（修改不会影响原数据）
• 现有代码无需修改即可安全使用

建议：考虑直接移除 deprecated 标记，因为当前实现是安全的。或者明确文档说明：

// GetParams returns a deep copy of params.
// Deprecated: Use CopyParams() for clarity. This method now also returns a copy.
func (c *URL) GetParams() url.Values {
    return c.CopyParams()
}

2. registry/directory/directory.go:372-388 - 重复代码

refreshAllInvokers 中的 configuratorURLs 处理逻辑与 convertUrl 存在重复，建议抽取公共方法：

func (dir *RegistryDirectory) processServiceEvent(event *registry.ServiceEvent) (*common.URL, bool) {
    // 返回 URL 和是否为 configurator 标志
}

3. common/url_test.go - 并发测试验证

新增的 TestMergeURLAttributeAccessRace 测试验证了并发访问，但仅运行 50ms，建议延长测试时间或增加迭代次数。

---

总体评价：修复方向正确，代码质量良好，建议优化 deprecated 方法的文档说明。

[No-SilverBullet]

LGTM :)

[AlexStocks]

文件都冲突了

[Alanxtl]

pls resolve file confilct

[AlexStocks]

please fix the file confliction.

[nagisa-kunhah]

please fix the file confliction.

Thanks for the reminder. I’ve been a bit busy these past few days, and I’ll resolve the file conflict shortly.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[AlexStocks]

[P1] 这里在 configurator-only 的 NotifyAll 场景下直接 return，只替换了 dir.configurators，但没有把新规则重新应用到已经缓存的 invoker。结果就是这批 override 要等下一次 provider 事件或配置中心事件触发后才会真正生效，Alan 之前提的那个语义问题还在。建议这里要么遍历 cacheInvokersMap 按原始 provider URL 重新 overrideUrl + doCacheInvoker，要么至少补一个覆盖现有 cached invoker 立即刷新行为的测试，确保纯 configurator 批次不会静默延迟。