fix(net): fix RejectedExecutionException during shutdown trxHandlePool#8
fix(net): fix RejectedExecutionException during shutdown trxHandlePool#80xbigapple wants to merge 1 commit intodevelopfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdded graceful shutdown mechanism to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes shutdown-time RejectedExecutionException in TransactionsMsgHandler by preventing new task submissions after close and changing executor shutdown ordering to stop the scheduler before the worker pool.
Changes:
- Add an
isClosedflag and guardprocessMessage/iteration to stop submitting work during shutdown. - Change shutdown order to terminate
smartContractExecutorbeforetrxHandlePool, and catchRejectedExecutionExceptionon submit. - Add a unit test to ensure
processMessage()does not throw afterclose().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| framework/src/main/java/org/tron/core/net/messagehandler/TransactionsMsgHandler.java | Adds close-gating and adjusts executor shutdown order to avoid rejected submissions during shutdown. |
| framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java | Adds a regression test ensuring processMessage is safe after handler close. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Re-check isClosed status during iteration to handle concurrent shutdown | ||
| if (isClosed) { | ||
| logger.warn("TransactionsMsgHandler is closed during processing, stop submit."); | ||
| break; | ||
| } |
There was a problem hiding this comment.
When isClosed flips to true (or trxHandlePool starts rejecting), this loop breaks early, but the earlier loop has already removed all tx items from peer.getAdvInvRequest(). That can leave some transactions neither queued nor requested anymore. To avoid silently dropping txs, consider removing items from advInvRequest only after a tx is successfully enqueued/submitted (or stop removing once shutdown is detected).
0xbigapple
force-pushed
the
fix/threadpool-submit-issue
branch
from
c3e2c56 to
213dd12
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0xbigapple
force-pushed
the
fix/threadpool-submit-issue
branch
from
4d5d454 to
595f6a8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java (1)
154-206: Add cleanup to avoid leaking executor threads.The test creates a
TransactionsMsgHandlerwhose field initializers instantiate real thread pools (trxHandlePoolandsmartContractExecutor). Without callingclose(), these executors remain running after the test completes, potentially causing resource leaks or test pollution.🧹 Proposed fix to add cleanup
`@Test` public void testHandleTransaction() throws Exception { TransactionsMsgHandler handler = new TransactionsMsgHandler(); + try { TronNetDelegate tronNetDelegate = Mockito.mock(TronNetDelegate.class); // ... existing test code ... Mockito.verify(peer).setBadPeer(true); Mockito.verify(peer).disconnect(Protocol.ReasonCode.BAD_TX); + } finally { + handler.close(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java` around lines 154 - 206, The test leaks executor threads because TransactionsMsgHandler's field initializers create real thread pools (trxHandlePool and smartContractExecutor) that are not shut down; fix by ensuring these are closed after the test—either call handler.close() (or if no public close exists, reflectively get and shutdown the trxHandlePool and smartContractExecutor fields) in a finally block at the end of testHandleTransaction or move cleanup into an `@After` method to always shut down those executors; reference TransactionsMsgHandler, trxHandlePool, smartContractExecutor, and close()/shutdown() when applying the change.framework/src/main/java/org/tron/core/net/messagehandler/TransactionsMsgHandler.java (1)
136-151: Consider adding try/catch for consistency (optional).While the shutdown order guarantees
trxHandlePoolis still running when this method executes, adding a try/catch forRejectedExecutionExceptionwould make the code more defensive and consistent withprocessMessage().🔧 Optional defensive improvement
private void handleSmartContract() { ExecutorServiceManager.scheduleWithFixedDelay(smartContractExecutor, () -> { try { while (queue.size() < MAX_SMART_CONTRACT_SUBMIT_SIZE && smartContractQueue.size() > 0) { TrxEvent event = smartContractQueue.take(); - ExecutorServiceManager.submit( - trxHandlePool, () -> handleTransaction(event.getPeer(), event.getMsg())); + try { + ExecutorServiceManager.submit( + trxHandlePool, () -> handleTransaction(event.getPeer(), event.getMsg())); + } catch (RejectedExecutionException e) { + logger.warn("Submit task to {} failed during shutdown", trxEsName); + break; + } } } catch (InterruptedException e) { logger.warn("Handle smart server interrupted"); Thread.currentThread().interrupt(); } catch (Exception e) { logger.error("Handle smart contract exception", e); } }, 1000, 20, TimeUnit.MILLISECONDS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/net/messagehandler/TransactionsMsgHandler.java` around lines 136 - 151, The scheduled task in handleSmartContract submits jobs to trxHandlePool without guarding for RejectedExecutionException; wrap the ExecutorServiceManager.submit(trxHandlePool, () -> handleTransaction(event.getPeer(), event.getMsg())) call in its own try/catch that catches RejectedExecutionException and logs a warning (including identifying info such as event.getPeer() or event) so the failure is visible and the loop can continue; keep existing InterruptedException and generic Exception handlers as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@framework/src/main/java/org/tron/core/net/messagehandler/TransactionsMsgHandler.java`:
- Around line 136-151: The scheduled task in handleSmartContract submits jobs to
trxHandlePool without guarding for RejectedExecutionException; wrap the
ExecutorServiceManager.submit(trxHandlePool, () ->
handleTransaction(event.getPeer(), event.getMsg())) call in its own try/catch
that catches RejectedExecutionException and logs a warning (including
identifying info such as event.getPeer() or event) so the failure is visible and
the loop can continue; keep existing InterruptedException and generic Exception
handlers as-is.
In
`@framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java`:
- Around line 154-206: The test leaks executor threads because
TransactionsMsgHandler's field initializers create real thread pools
(trxHandlePool and smartContractExecutor) that are not shut down; fix by
ensuring these are closed after the test—either call handler.close() (or if no
public close exists, reflectively get and shutdown the trxHandlePool and
smartContractExecutor fields) in a finally block at the end of
testHandleTransaction or move cleanup into an `@After` method to always shut down
those executors; reference TransactionsMsgHandler, trxHandlePool,
smartContractExecutor, and close()/shutdown() when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a71c64cd-51ca-44f4-921c-93b3f371bb29
📒 Files selected for processing (2)
framework/src/main/java/org/tron/core/net/messagehandler/TransactionsMsgHandler.javaframework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java
0xbigapple
force-pushed
the
fix/threadpool-submit-issue
branch
from
595f6a8 to
b530d30
Compare
What does this PR do?
fix RejectedExecutionException during shutdown trxHandlePool
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Fixes RejectedExecutionException during shutdown by blocking new submissions, shutting down the scheduler before the worker pool, and clearing queues. Ensures clean shutdown after close, ignores new messages, and drops only queued smart‑contract tasks.
smartContractExecutorfirst, thentrxHandlePool; clearsmartContractQueueand internalqueue.trxHandlePoolin try/catch RejectedExecutionException; stop submitting on first rejection.Written for commit b530d30. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests