[Enhancement] NettyEventExecutor shutdown waits for the eventQueue poll timeout because wakeup() cannot release it
- 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
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