qianye1001 opened a new issue, #11161: URL: https://github.com/apache/rocketmq/issues/11161
### Before Creating the Bug Report - [x] I searched the [GitHub issues](https://github.com/apache/rocketmq/issues) and found no similar report. - [x] I reviewed the code on the current `develop` branch. ### Runtime platform environment All platforms (client side, `NettyRemotingClient`). ### RocketMQ version `develop` (reproduced by reading the current source). ### Component remoting ### Describe what happened There are two related defects in `org.apache.rocketmq.remoting.netty.NettyRemotingClient` on the current `develop` branch. #### Bug 1 — `closeChannel(String addr, Channel channel)` no longer evicts the channel-table entry ```java if (null == prevCW) { ... removeItemFromTable = false; } else if (prevCW.isWrapperOf(channel)) { // <-- inverted LOGGER.info("... has been closed before, and has been created again, nothing to do.", ...); removeItemFromTable = false; } ``` `isWrapperOf(channel)` is true exactly when the wrapper currently stored under `addr` wraps *this* channel — i.e. the normal case of closing the live channel. In that case the code takes the "has been created again, nothing to do" branch and sets `removeItemFromTable = false`, so the entry is **not** removed from `channelTables`. Before #8366 the condition was `else if (prevCW.getChannel() != channel)`, which is the logical negation of `isWrapperOf(channel)`. #8366 replaced the expression with `isWrapperOf(channel)` but did not re-negate it, so the branch semantics and the accompanying log message are now inverted. Effects: - The eviction block inside `closeChannel(addr, channel)` becomes effectively dead for the common path. Note the two conditions are mutually exclusive: the block is only reached when the wrapper is *not* this channel, yet `channelWrapper.tryClose(channel)` only returns true when the wrapper *is* this channel. - The map entry is only cleaned up indirectly: the trailing `RemotingHelper.closeChannel(channel)` closes the socket, which fires the pipeline close event and reaches the single-argument `closeChannel(Channel)` (that method does not have the inverted branch). So the entry is eventually removed, but via a fragile pipeline-event detour rather than the intended eager removal, and the misleading "created again, nothing to do" log fires on every ordinary close. #### Bug 2 — `ChannelWrapper.close()` acquires locks in the order opposite to the rest of the class ```java public void close() { try { lock.writeLock().lock(); // wrapper write lock if (channelFuture != null) { closeChannel(channelFuture.channel()); // -> acquires lockChannelTables } if (channelToClose != null) { closeChannel(channelToClose.channel()); } } finally { lock.writeLock().unlock(); } } ``` Every other path takes `lockChannelTables` first and then a wrapper lock: - `createChannelAsync`: `lockChannelTables` → `ChannelWrapper.getChannelFuture()`/`isOK()` (wrapper read lock) - `closeChannel(addr, channel)` / `closeChannel(channel)`: `lockChannelTables` → `ChannelWrapper.tryClose()` (wrapper read lock) `ChannelWrapper.close()` reverses that order (`wrapper write lock` → `lockChannelTables`), which is a classic AB-BA lock-ordering inversion. In practice it does not hang forever because `lockChannelTables` is acquired with `tryLock(3000ms)`, but the reverse ordering can still stall a thread — and block everything else waiting on `lockChannelTables`, including the single-threaded selector reaching `closeChannel` from a pipeline event — for up to the 3s timeout. `close()` is invoked from `shutdown()` and from `updateNameServerAddressList()`. ### Describe what you expected to happen - `closeChannel(addr, channel)` should evict the table entry when the stored wrapper wraps the channel being closed, and should leave it alone only when the wrapper has already been recreated for a different channel. - `ChannelWrapper.close()` should not hold a wrapper lock while acquiring `lockChannelTables`, so that the whole client obeys a single consistent lock order (`namesrvChannelLock` → `lockChannelTables` → wrapper lock). ### How to reproduce Bug 1 is directly observable in a unit test: install a `ChannelWrapper` for an address into `channelTables`, call `closeChannel(addr, wrapper.channel())`, and assert the entry is gone. On current `develop` the entry remains. ### Additional context I have a fix ready (restore the negation for Bug 1; rework `close()` to snapshot the channel futures under the read lock and then call `closeChannel(...)` without holding any wrapper lock, still closing both `channelFuture` and `channelToClose`) plus regression tests, and will open a PR referencing this issue. -- 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]
