Optimize shutdownScheduledExecutorService to ensure complete task termination #9872
Conversation
…mination Change-Id: I288dcab67dcdb58acf154fd9d3f5f5c2f85fb32d Co-developed-by: Cursor <[email protected]>
…mination Change-Id: Ib637e6b2eb79cc108cdb6eee9a24f492f5f0c129
There was a problem hiding this comment.
Pull request overview
This PR enhances the robustness of the ScheduledExecutorService shutdown process in the broker by implementing a multi-stage termination strategy with better timeout handling and forced shutdown capabilities.
Key changes:
- Increased graceful shutdown timeout from 5 to 60 seconds with fallback to forced shutdown
- Added forced shutdown with
shutdownNow()when graceful termination times out, including logging of cancelled pending tasks - Enhanced thread interruption handling during shutdown
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java | Implements multi-stage executor shutdown with extended timeout (60s), forced termination on timeout, and secondary wait period after forced shutdown |
| broker/src/main/java/org/apache/rocketmq/broker/util/HookUtils.java | Minor log message wording improvement, removing redundant "so" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!scheduledExecutorService.awaitTermination(5000, TimeUnit.MILLISECONDS)) { | ||
| BrokerController.LOG.error("ScheduledExecutorService did not terminate after forced shutdown"); | ||
| } else { | ||
| BrokerController.LOG.info("ScheduledExecutorService terminated successfully after forced shutdown"); | ||
| } | ||
| } else { | ||
| BrokerController.LOG.debug("ScheduledExecutorService terminated gracefully"); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| // If interrupted during waiting, force shutdown | ||
| BrokerController.LOG.warn("shutdown ScheduledExecutorService was Interrupted, forcing shutdown...", e); | ||
| scheduledExecutorService.shutdownNow(); |
There was a problem hiding this comment.
The second awaitTermination call after shutdownNow() can throw InterruptedException, but this exception is not caught here. If interrupted, the exception will propagate to the outer catch block at line 1691, which will call shutdownNow() again (redundant since it was already called) and set the interrupt flag. However, the success log at line 1686 would never be reached if interrupted.
Consider wrapping this second awaitTermination call in its own try-catch block to handle interruption properly, or document that interruption during forced shutdown is acceptable.
| if (!scheduledExecutorService.awaitTermination(5000, TimeUnit.MILLISECONDS)) { | |
| BrokerController.LOG.error("ScheduledExecutorService did not terminate after forced shutdown"); | |
| } else { | |
| BrokerController.LOG.info("ScheduledExecutorService terminated successfully after forced shutdown"); | |
| } | |
| } else { | |
| BrokerController.LOG.debug("ScheduledExecutorService terminated gracefully"); | |
| } | |
| } catch (InterruptedException e) { | |
| // If interrupted during waiting, force shutdown | |
| BrokerController.LOG.warn("shutdown ScheduledExecutorService was Interrupted, forcing shutdown...", e); | |
| scheduledExecutorService.shutdownNow(); | |
| try { | |
| if (!scheduledExecutorService.awaitTermination(5000, TimeUnit.MILLISECONDS)) { | |
| BrokerController.LOG.error("ScheduledExecutorService did not terminate after forced shutdown"); | |
| } else { | |
| BrokerController.LOG.info("ScheduledExecutorService terminated successfully after forced shutdown"); | |
| } | |
| } catch (InterruptedException ie) { | |
| BrokerController.LOG.warn("Interrupted while waiting for ScheduledExecutorService to terminate after forced shutdown", ie); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } else { | |
| BrokerController.LOG.debug("ScheduledExecutorService terminated gracefully"); | |
| } | |
| } catch (InterruptedException e) { | |
| // If interrupted during initial waiting, force shutdown | |
| BrokerController.LOG.warn("shutdown ScheduledExecutorService was Interrupted during initial wait, forcing shutdown...", e); |
| // Wait for tasks to complete, at most 60 seconds | ||
| if (!scheduledExecutorService.awaitTermination(60000, TimeUnit.MILLISECONDS)) { |
There was a problem hiding this comment.
[nitpick] The timeout has been increased from 5 seconds to 60 seconds per executor shutdown. Since shutdownScheduledExecutorService is called three times during broker shutdown (lines 1508, 1603, 1604), this could potentially extend total shutdown time from 15 seconds to 180 seconds (3 minutes) in the worst-case scenario where all three executors time out.
While this provides more time for graceful task completion, consider:
- Making the timeout configurable via
BrokerConfigto allow operators to tune it based on their workload - Adding metrics or logging to track actual shutdown durations to identify if such long timeouts are necessary
- Documenting the maximum expected shutdown time for operators
Note: The test at BrokerShutdownTest.testBrokerGracefulShutdown currently expects shutdown within 40 seconds, which would need to be updated if this long timeout is retained.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9872 +/- ##
=============================================
- Coverage 48.39% 48.29% -0.10%
+ Complexity 12280 12257 -23
=============================================
Files 1314 1314
Lines 93894 93905 +11
Branches 12046 12049 +3
=============================================
- Hits 45436 45352 -84
- Misses 42852 42930 +78
- Partials 5606 5623 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Which Issue(s) This PR Fixes
Fixes #issue_id
Brief Description
Optimize shutdownScheduledExecutorService to ensure complete task termination
How Did You Test This Change?