You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "qianye1001 (via GitHub)" <gi...@apache.org> on 2026/09/14 07:38:46 UTC
[PR] [ISSUE #11161] Fix closeChannel table eviction and ChannelWrapper.close lock ordering [rocketmq]
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #11161] Fix closeChannel table eviction and ChannelWrapper.close lock ordering [rocketmq]
Posted by "lollipopjin (via GitHub)" <gi...@apache.org>.
lollipopjin merged PR #11162:
URL: https://github.com/apache/rocketmq/pull/11162
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #11161] Fix closeChannel table eviction and ChannelWrapper.close lock ordering [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11162:
URL: https://github.com/apache/rocketmq/pull/11162#issuecomment-5661000412
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/11162?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `66.66667%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 49.36%. Comparing base ([`290d440`](https://app.codecov.io/gh/apache/rocketmq/commit/290d440e7d7beefb18740968f7129694fa016537?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ef3d889`](https://app.codecov.io/gh/apache/rocketmq/commit/ef3d889a58ae6acc21502139b68b9848e87106c1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/rocketmq/pull/11162?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...e/rocketmq/remoting/netty/NettyRemotingClient.java](https://app.codecov.io/gh/apache/rocketmq/pull/11162?src=pr&el=tree&filepath=remoting%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fremoting%2Fnetty%2FNettyRemotingClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L05ldHR5UmVtb3RpbmdDbGllbnQuamF2YQ==) | 66.66% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/11162?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/11162?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org