Copilot commented on code in PR #11005:
URL: https://github.com/apache/rocketmq/pull/11005#discussion_r3922839870
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -775,6 +781,16 @@ public void putNettyEvent(final NettyEvent event) {
}
}
+ @Override
+ public void wakeup() {
+ super.wakeup();
+ // This service blocks in eventQueue.poll(...) instead of
waitForRunning, so the unpark
+ // done by super.wakeup() cannot release it and shutdown would
wait for the poll timeout
+ // to expire. Offer a sentinel to make the poll return at once,
letting the loop observe
+ // the stopped flag immediately.
+ this.eventQueue.offer(this.wakeupEvent);
Review Comment:
NettyEventExecutor.wakeup() now always enqueues the sentinel, even when
super.wakeup() is a no-op (hasNotified already true) or when the executor isn’t
stopping. This bypasses ServiceThread’s throttling and can add unnecessary
sentinel entries if wakeup() is ever called repeatedly.
##########
remoting/src/test/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstractTest.java:
##########
@@ -168,4 +169,24 @@ public void operationFail(Throwable throwable) {
semaphore.acquire(1);
assertThat(semaphore.availablePermits()).isEqualTo(0);
}
+
+ @Test
+ public void testNettyEventExecutorShutdownDoesNotWaitForPollTimeout()
throws InterruptedException {
+ // NettyEventExecutor blocks in eventQueue.poll(3000ms) rather than
waitForRunning, so the
+ // wakeup() performed by ServiceThread.shutdown() cannot interrupt it:
the thread only
+ // notices the stopped flag once the poll expires, making every
shutdown wait up to 3s.
+ // A broker or client shutdown pays this for each remoting instance it
owns.
+ NettyRemotingAbstract remoting = new NettyRemotingClient(new
NettyClientConfig());
+ remoting.nettyEventExecutor.start();
+
+ // let the thread reach the blocking poll
+ TimeUnit.MILLISECONDS.sleep(300);
+
+ long begin = System.currentTimeMillis();
+ remoting.nettyEventExecutor.shutdown();
+ long elapsed = System.currentTimeMillis() - begin;
+
+ assertThat(remoting.nettyEventExecutor.isStopped()).isTrue();
+ assertThat(elapsed).isLessThan(1000);
+ }
Review Comment:
This test relies on a fixed sleep and System.currentTimeMillis() for timing.
The sleep can race (allowing a false pass if the worker hasn’t entered the 3s
poll yet), and the test also doesn’t clean up the NettyRemotingClient resources
it constructs. Prefer waiting until the NettyEventExecutor thread is in
TIMED_WAITING and measure duration with nanoTime, then ensure client shutdown
in a finally block.
--
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]
