Skip to content

[ISSUE #10898] Wake NettyEventExecutor during shutdown - #10910

Open
1ASunny wants to merge 1 commit into
apache:developfrom
1ASunny:fix-10898-netty-event-shutdown
Open

[ISSUE #10898] Wake NettyEventExecutor during shutdown#10910
1ASunny wants to merge 1 commit into
apache:developfrom
1ASunny:fix-10898-netty-event-shutdown

Conversation

@1ASunny

@1ASunny 1ASunny commented Aug 11, 2026

Copy link
Copy Markdown

Which Issue(s) This PR Fixes

Brief Description

NettyEventExecutor.wakeup() now offers a private sentinel event after delegating to ServiceThread.wakeup(). This releases a worker blocked in the event queue's three-second poll so shutdown can observe the stopped flag immediately. The dispatch loop skips the sentinel by identity, leaving normal channel events unchanged.

The regression coverage verifies both prompt shutdown while the executor is blocked in the timed poll and that the private wakeup sentinel is never dispatched to the channel listener.

How Did You Test This Change?

  • NettyRemotingAbstractTest: 7 tests passed.
  • Checkstyle passed with 0 violations.
  • SpotBugs passed with 0 issues.
  • git diff --check passed.
  • The full common + remoting suite was attempted on Windows with JDK 17, but did not complete because the existing RemotingServerTest#testInvokeAsync waits indefinitely in CountDownLatch.await(). The focused test class and unrelated compatibility test both pass.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Looks good.


Automated review by github-manager-bot

@fuyou001

Copy link
Copy Markdown
Contributor

[High] Coalesce the wake-up sentinel to preserve notification and bounded-queue semantics

ServiceThread.wakeup() intentionally coalesces repeated notifications through hasNotified, but this override unconditionally executes eventQueue.offer(wakeupEvent), even when the parent wakeup has already become a no-op. Because eventQueue is an unbounded LinkedBlockingQueue and putNettyEvent() starts dropping normal events once the queue exceeds 10,000 entries, repeated or concurrent wakeup() calls, inherited makeStop() calls, or restart races can retain an unbounded number of sentinel nodes and eventually cause real CLOSE, IDLE, or EXCEPTION events to be dropped. That can skip broker, client, route, or heartbeat cleanup.

I reproduced this on the current head: 10,001 wakeup() calls resulted in 10,001 queued sentinel entries, whereas the parent notification contract coalesces them.

Please guard the control marker with an atomic pending state so at most one sentinel can be queued, and reset that state safely when the marker is consumed, including shutdown-before-run and restart races. A regression test should block the listener, issue more than 10,000 serial and concurrent wakeup()/makeStop() calls, enqueue a real CLOSE event, release the listener, and assert that the CLOSE event is delivered exactly once in FIFO order while the number of control markers remains bounded. Please also cover shutdown(false), shutdown(true), repeated stop, and stop/restart.

@1ASunny

1ASunny commented Aug 13, 2026

Copy link
Copy Markdown
Author

[High] Coalesce the wake-up sentinel to preserve notification and bounded-queue semantics

ServiceThread.wakeup() intentionally coalesces repeated notifications through hasNotified, but this override unconditionally executes eventQueue.offer(wakeupEvent), even when the parent wakeup has already become a no-op. Because eventQueue is an unbounded LinkedBlockingQueue and putNettyEvent() starts dropping normal events once the queue exceeds 10,000 entries, repeated or concurrent wakeup() calls, inherited makeStop() calls, or restart races can retain an unbounded number of sentinel nodes and eventually cause real CLOSE, IDLE, or EXCEPTION events to be dropped. That can skip broker, client, route, or heartbeat cleanup.

I reproduced this on the current head: 10,001 wakeup() calls resulted in 10,001 queued sentinel entries, whereas the parent notification contract coalesces them.

Please guard the control marker with an atomic pending state so at most one sentinel can be queued, and reset that state safely when the marker is consumed, including shutdown-before-run and restart races. A regression test should block the listener, issue more than 10,000 serial and concurrent wakeup()/makeStop() calls, enqueue a real CLOSE event, release the listener, and assert that the CLOSE event is delivered exactly once in FIFO order while the number of control markers remains bounded. Please also cover shutdown(false), shutdown(true), repeated stop, and stop/restart.

Thanks for catching this. I reproduced the issue and confirmed that repeated wakeups can enqueue unbounded sentinel events. I’m working on coalescing the sentinel with an atomic pending state and adding lifecycle and concurrency regression tests. I’ll push an update shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] NettyEventExecutor shutdown waits for the eventQueue poll timeout because wakeup() cannot release it

3 participants