apache / apache/rocketmq

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

Open
#10,898 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
22.6k
Forks
12k
Avg merge
3d 1h
Merged PRs (30d)
27

Description

### Before Creating the Enhancement Request

- [x] I have confirmed that this should be classified as an enhancement rather than a bug/feature.

### Summary

`NettyEventExecutor` blocks in `eventQueue.poll(3000, TimeUnit.MILLISECONDS)`, but `ServiceThread.shutdown()` signals a stop through the `stopped` flag plus `wakeup()`. `wakeup()` only unparks a thread waiting inside `waitForRunning`; it cannot release a thread blocked on a `LinkedBlockingQueue`. The thread therefore keeps waiting until the poll expires before it observes the stopped flag, so every shutdown of a remoting instance waits up to 3 seconds for nothing.

A single `BrokerController.shutdown()` owns several remoting instances, so the cost adds up. Local run of `BrokerShutdownTest` on `develop`:

```
join thread[NettyEventExecutor], elapsed time: 2993ms, join time:90000ms
join thread[NettyEventExecutor], elapsed time: 2999ms, join time:90000ms
join thread[NettyEventExecutor], elapsed time: 2999ms, join time:90000ms
```

Three instances, roughly 9 seconds of pure waiting per broker shutdown.

### Motivation

Shutdown latency is paid in production on every graceful restart, and in CI on every test that starts and stops a broker or client. It also contributes to #10823: `BrokerShutdownTest` performs four full start/stop cycles, and this waiting is a large part of why the class exceeds the Bazel small test timeout.

### Describe the Solution You'd Like

Override `wakeup()` in `NettyEventExecutor` to offer a sentinel event to `eventQueue` after delegating to `super.wakeup()`, so the pending `poll` returns immediately and the loop re-checks the stopped flag. The dispatch loop skips the sentinel by reference, so no listener callback is triggered and `NettyEventType` does not need a new constant.

Measured locally with this change:

| | before | after |
|---|---|---|
| `join thread[NettyEventExecutor]` | ~3000 ms each | 0 ms each |
| `BrokerShutdownTest` | 71.98 s | 53.63 s |
| `NettyRemotingAbstractTest` | 3.76 s | 1.08 s |

`remoting` is green at 175/175 with zero Checkstyle violations.

### Describe Alternatives You've Considered

Shortening the poll timeout, for example to 500 ms. That reduces the worst-case wait but does not remove it, and it multiplies idle wakeups for a queue that is empty most of the time.

Interrupting the thread on shutdown. `ServiceThread` deliberately keeps `shutdown()` free of interruption and offers `shutdown(true)` for callers that want it, so relying on interruption here would work against that design.

### Additional Context

The sentinel is a private field compared by reference, so it cannot collide with a real event, and `putNettyEvent` is left untouched to keep the existing queue-size guard for genuine channel events.

Contributor guide

Open the contributing guide

Research direction

Start at NettyEventExecutor and inspect its eventQueue poll loop together with the inherited ServiceThread wakeup and shutdown behavior. Use BrokerShutdownTest and NettyRemotingAbstractTest as regression checks; done means shutdown no longer waits for the poll timeout and the remoting tests remain green.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, distributed-systems
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.