[ISSUE #11161] Fix closeChannel table eviction and ChannelWrapper.close lock ordering

[qianye1001]

Which Issue(s) This PR Fixes

Fixes #11161.

Brief Description

Two related fixes in NettyRemotingClient.

Bug 1 — closeChannel(String addr, Channel channel) no longer evicts the table entry. #8366 replaced the original condition else if (prevCW.getChannel() != channel) with else if (prevCW.isWrapperOf(channel)), which is its un-negated form. As a result the "has been created again, nothing to do" branch now triggers on the normal close path (the stored wrapper wraps this very channel), sets removeItemFromTable = false, and the entry is not evicted. The eviction block also became mutually exclusive with its own tryClose(channel) guard, so it is effectively dead; the entry only gets cleaned up indirectly when the trailing RemotingHelper.closeChannel(channel) fires the pipeline close event into the single-arg closeChannel(Channel). This restores the negation (!prevCW.isWrapperOf(channel)) so the entry is evicted on normal close and preserved only when the wrapper has been recreated for a different channel.

Bug 2 — ChannelWrapper.close() inverted the lock order. It held the wrapper write lock while calling closeChannel(...), which acquires lockChannelTables. Every other path takes lockChannelTables first and then a wrapper lock (createChannelAsync via getChannelFuture()/isOK(), closeChannel via tryClose()), so this was an AB-BA inversion. It did not hang permanently because lockChannelTables uses tryLock(3000ms), but the reverse order can stall a thread — and everything else waiting on lockChannelTables, including the selector thread reaching closeChannel from a pipeline event — for up to 3s. close() now snapshots both channel futures under the read lock, releases it, then calls closeChannel(...) without holding any wrapper lock, so the whole client obeys one consistent order (namesrvChannelLock → lockChannelTables → wrapper lock). Both channelFuture and channelToClose are still closed so the reconnect leftover channel is not leaked, and the two-arg closeChannel(channelAddress, ...) is used so the table is looked up by address instead of a linear scan.

How Did You Test This Change?

JDK 11:

mvn -B -pl remoting -am -Dmaven.gitcommitid.skip=true \
  -Dtest=NettyRemotingClientTest,NettyRemotingClientCloseChannelTest \
  -Dsurefire.failIfNoSpecifiedTests=false test

Added NettyRemotingClientCloseChannelTest covering both closeChannel(addr, channel) branches: the entry is evicted when the wrapper wraps the closed channel, and preserved when the wrapper has been recreated for a different channel. The first assertion fails on the current (inverted) code and passes with the fix. Existing NettyRemotingClientTest (16 tests) continues to pass.

[RockteMQ-AI]

Summary

Excellent fix for two related concurrency issues in NettyRemotingClient.

Bug 1 (condition inversion): The closeChannel(addr, channel) method had an inverted condition — prevCW.isWrapperOf(channel) instead of !prevCW.isWrapperOf(channel). This caused the channel table entry to not be evicted on normal close, leading to stale entries. The fix correctly restores the negation.

Bug 2 (lock ordering): ChannelWrapper.close() held the wrapper write lock while calling closeChannel(), which acquires lockChannelTables. This inverted the lock order used everywhere else (lockChannelTables → wrapper lock), creating an AB-BA inversion. The fix correctly snapshots the channel futures under a read lock, releases it, then closes without holding any wrapper lock.

The use of the two-arg closeChannel(channelAddress, channel) also avoids a linear scan, which is a nice optimization.

Tests cover both branches of the fix — entry eviction when the wrapper matches, and preservation when the wrapper has been recreated. Well-structured and clear.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.36%. Comparing base (290d440) to head (ef3d889).

Files with missing lines	Patch %	Lines
...e/rocketmq/remoting/netty/NettyRemotingClient.java	66.66%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #11162      +/-   ##
=============================================
- Coverage      49.42%   49.36%   -0.07%     
+ Complexity     14243    14230      -13     
=============================================
  Files           1390     1390              
  Lines         103130   103132       +2     
  Branches       13485    13485              
=============================================
- Hits           50974    50908      -66     
- Misses         45989    46050      +61     
- Partials        6167     6174       +7     

☔ View full report in Codecov by Harness.
📢 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.

[lollipopjin]

LGTM