qianye1001 opened a new pull request, #11162:
URL: https://github.com/apache/rocketmq/pull/11162
### 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:
```shell
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.
--
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]
