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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a closed-state flag to TransactionsMsgHandler that blocks new message processing and task submissions during shutdown; adjusted shutdown order to stop the smart-contract scheduler before the transaction worker pool and added handling for RejectedExecutionException. New unit tests exercise lifecycle and transaction handling behaviors. 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
0xbigapple
force-pushed
the
fix/threadpool-submit-issue
branch
from
b530d30 to
2e3164c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
| if (isClosed) { | ||
| logger.warn("TransactionsMsgHandler is closed, drop message"); | ||
| return; | ||
| } | ||
| TransactionsMessage transactionsMessage = (TransactionsMessage) msg; |
There was a problem hiding this comment.
processMessage returns immediately when isClosed is true, before removing the received transactions from peer.getAdvInvRequest(). If close() is called while peers are still connected, those request entries can linger and later trigger TIME_OUT disconnects in PeerStatusCheck even though the peer actually responded. Consider still doing the minimal request cleanup (remove advInvRequest entries for txs in the message) while skipping validation/submission when closed, or clearing relevant state as part of shutdown.
| if (isClosed) { | |
| logger.warn("TransactionsMsgHandler is closed, drop message"); | |
| return; | |
| } | |
| TransactionsMessage transactionsMessage = (TransactionsMessage) msg; | |
| TransactionsMessage transactionsMessage = (TransactionsMessage) msg; | |
| if (isClosed) { | |
| for (Transaction trx : transactionsMessage.getTransactions().getTransactionsList()) { | |
| Item item = new Item(new TransactionMessage(trx).getMessageId(), InventoryType.TRX); | |
| peer.getAdvInvRequest().remove(item); | |
| } | |
| logger.warn("TransactionsMsgHandler is closed, drop message"); | |
| return; | |
| } |
| ExecutorServiceManager.submit( | ||
| trxHandlePool, () -> handleTransaction(peer, new TransactionMessage(trx))); | ||
| } catch (RejectedExecutionException e) { | ||
| logger.warn("Submit task to {} failed", trxEsName); |
There was a problem hiding this comment.
The RejectedExecutionException catch logs only a generic message and drops the exception details. Including e (or at least e.getMessage()) in the log will make shutdown/rejection issues diagnosable in production, especially since this path is explicitly handling a failure mode.
| logger.warn("Submit task to {} failed", trxEsName); | |
| logger.warn("Submit task to {} failed", trxEsName, e); |
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
Prevents RejectedExecutionException during shutdown by blocking new submissions, stopping the scheduler before the worker pool, and clearing pending queues for a clean close. New messages are ignored after close; mid-batch processing stops safely; only pending tasks are dropped.
isClosed(early return inprocessMessage, re-check during iteration).smartContractExecutorbeforetrxHandlePool; clearsmartContractQueueand internalqueue.Written for commit 2e3164c. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests