fix(proxy): unregister all publishing topics on gRPC producer termination - #10665
fix(proxy): unregister all publishing topics on gRPC producer termination#10665btlqql wants to merge 1 commit into
Conversation
…tion In notifyClientTermination, grpcChannelManager.removeChannel(clientId) was called inside the for-each loop over publishing topics. Since removeChannel removes and returns the channel on the first call, subsequent iterations received null and skipped unregistration. Only the first topic was ever unregistered. Move removeChannel before the loop so the channel is removed once and all publishing topics are properly unregistered. Fixes apache#10661 Signed-off-by: btlqql <2977859784@qq.com>
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes a bug where gRPC producer termination only unregisters the first publishing topic from the broker, leaving stale producer registrations for remaining topics.
Analysis
Root Cause: In the original code, grpcChannelManager.removeChannel(clientId) was called inside the for loop that iterates over publishing topics. Since removeChannel removes and returns the channel on the first call, subsequent iterations return null. This means:
- 1st topic: channel removed, producer unregistered ✓
- 2nd+ topics: channel is
null,unRegisterProducerskipped ✗
Fix: Move removeChannel(clientId) before the loop, store the result in a local variable, and reuse it for all topic unregistrations. This is correct and minimal.
Observations
-
Correctness ✅ — The fix properly ensures all publishing topics are unregistered. The channel is removed exactly once and reused for every
unRegisterProducercall. -
Consistency ✅ — The new code structure matches the existing pattern used for consumer cases (
PUSH_CONSUMER,SIMPLE_CONSUMER) in the same method, which also callremoveChannelonce before processing. -
Scope ✅ — Minimal change (5+/5-) in a single file. No behavioral change to individual unregister semantics, only fixes the control flow ordering.
-
Edge case — If
getPublishing().getTopicsList()is empty, the channel is still removed (correct cleanup behavior). IfremoveChannelreturnsnull(channel already removed), no unregistration is attempted (safe).
Verdict
Approve. Clean, correct fix for a real resource leak issue. Well-scoped and consistent with existing code patterns.
|
Thanks for the fix. This PR duplicates #10662, which has already been merged into The merged change also includes regression coverage for unregistering multiple publishing topics with the same |
痛点
gRPC producer 终止时,notifyClientTermination 中 grpcChannelManager.removeChannel(clientId) 被放在 for 循环内部。removeChannel 第一次调用时移除并返回 channel,后续迭代返回 null,导致只有第一个 publishing topic 被注销,其余 topic 的 producer 注册信息残留在 broker 端。
Fixes #10661
已实现
验证
Signed-off-by: btlqql 2977859784@qq.com